* Re: [PATCH 3/6] Bluetooth: Implement the first SMP commands
From: Luiz Augusto von Dentz @ 2010-10-26 15:22 UTC (permalink / raw)
To: Ville Tervo
Cc: ext Gustavo F. Padovan, Anderson Briglia,
linux-bluetooth@vger.kernel.org, Vinicius Costa Gomes
In-Reply-To: <20101026092623.GC15050@null>
Hi,
On Tue, Oct 26, 2010 at 2:26 AM, Ville Tervo <ville.tervo@nokia.com> wrote:
> On Mon, Oct 25, 2010 at 03:03:56PM +0200, ext Gustavo F. Padovan wrote:
>> Hi Vinicius,
>>
>> * Anderson Briglia <anderson.briglia@openbossa.org> [2010-10-22 19:56:57 -0400]:
>>
>> > From: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
>> >
>> > These simple commands will allow the SMP procedure to be started
>> > and terminated with a not supported error. This is the first step
>> > toward something useful.
>> >
>> > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
>> > ---
>> > net/bluetooth/l2cap.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++
>> > 1 files changed, 117 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>> > index 1ac44f4..ba87c84 100644
>> > --- a/net/bluetooth/l2cap.c
>> > +++ b/net/bluetooth/l2cap.c
>> > @@ -54,6 +54,7 @@
>> > #include <net/bluetooth/bluetooth.h>
>> > #include <net/bluetooth/hci_core.h>
>> > #include <net/bluetooth/l2cap.h>
>> > +#include <net/bluetooth/smp.h>
>> >
>> > #define VERSION "2.15"
>> >
>> > @@ -307,6 +308,85 @@ static void l2cap_chan_del(struct sock *sk, int err)
>> > }
>> > }
>> >
>> > +static struct sk_buff *smp_build_cmd(struct l2cap_conn *conn, u8 code,
>> > + u16 dlen, void *data)
>>
>> Call this l2cap_smp_build_cmd()
>
> Should the whole smp stuff be in separate file (smp.c)? It's not a l2cap feature but a
> protocol using l2cap. In that case smp_build_cmd would be good name.
+1
It is also much better for maintenance and development since there is
less patches touching the l2cap.c so less chances of conflicts,
rebases and regressions on l2cap.
--
Luiz Augusto von Dentz
Computer Engineer
^ permalink raw reply
* Re: [PATCH] Fix cache is invalid on empty listing response
From: Luiz Augusto von Dentz @ 2010-10-26 15:01 UTC (permalink / raw)
To: Dmitriy Paliy; +Cc: linux-bluetooth
In-Reply-To: <1288082028-10014-1-git-send-email-dmitriy.paliy@nokia.com>
Hi,
On Tue, Oct 26, 2010 at 1:33 AM, Dmitriy Paliy <dmitriy.paliy@nokia.com> wrote:
> Fix cache marked as invalid when no data available in response to get phone
> book listing. This fixes obexd crash in 3-way calling scenario. Cache is
> created when there is second incoming call. If the call is rejected and
> retried again, valid empty cache is sent over obex stream that causes obexd
> crash. With this fix, the stream is aborted in such scenario. A new attempt
> to create cache is carried out on each new subsequent incoming call.
> ---
> plugins/pbap.c | 6 +++++-
> 1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/plugins/pbap.c b/plugins/pbap.c
> index 11cb678..f7f3897 100644
> --- a/plugins/pbap.c
> +++ b/plugins/pbap.c
> @@ -407,7 +407,11 @@ static void cache_ready_notify(void *user_data)
> (const char *) pbap->params->searchval);
>
> if (sorted == NULL) {
> - pbap->cache.valid = TRUE;
> + /* When no data available from tracker, cache marked as
> + * invalid (new one will be tried to be created next time),
> + * no such file or directory error is set, and obex stream
> + * is aborted */
> + pbap->cache.valid = FALSE;
> obex_object_set_io_flags(pbap, G_IO_ERR, -ENOENT);
> return;
> }
We discussed about this offline and apparently when cache.valid is set
to FALSE it will create a new cache for the next requests so we need
to free the current cache otherwise it will leak.
--
Luiz Augusto von Dentz
Computer Engineer
^ permalink raw reply
* Re: [PATCH 2/2] Bluetooth: Fix system crash bug of no send queue protect
From: haijun liu @ 2010-10-26 14:36 UTC (permalink / raw)
To: Haijun Liu; +Cc: linux-bluetooth
In-Reply-To: <1287644956-12602-2-git-send-email-haijun.liu@atheros.com>
Sorry, this patch has been discarded, please look at
[PATCH 1/2 v2] Bluetooth: Fix system crash caused by del_timer()
[PATCH 2/2 v2] Bluetooth: Fix system crash bug of no send queue protect
--
Haijun Liu
^ permalink raw reply
* Re: [PATCH 1/2] Bluetooth: Fix system crash caused by del_timer()
From: haijun liu @ 2010-10-26 14:33 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Haijun Liu, linux-bluetooth
In-Reply-To: <1288103093.3613.21.camel@aeonflux>
Hi Marcel,
>> At UPF37, during test session of bt3.0 with Marvel, found that in
>> l2cap_chan_del() using del_timer() caused l2cap_monitor_timeout()
>> be called after the sock was freed, so it raised a system crash.
>> So I just replaced del_timer() with del_timer_sync() to solve it.
>>
>> Signed-off-by: Haijun Liu <haijun.liu@atheros.com>
>
> looks good to me and if Gustavo agrees, then
>
> Acked-by: Marcel Holtmann <marcel@holtmann.org>
>
Sorry, I should note this patch has been discard, please look at
<[PATCH 1/2 v2] Bluetooth: Fix system crash caused by del_timer() >
<[PATCH 2/2 v2] Bluetooth: Fix system crash bug of no send queue protect>
Thank you.
--
Haijun Liu
^ permalink raw reply
* Re: [PATCH 1/2] Bluetooth: Fix system crash caused by del_timer()
From: Marcel Holtmann @ 2010-10-26 14:24 UTC (permalink / raw)
To: Haijun Liu; +Cc: linux-bluetooth
In-Reply-To: <1287644956-12602-1-git-send-email-haijun.liu@atheros.com>
Hi Haijun,
> At UPF37, during test session of bt3.0 with Marvel, found that in
> l2cap_chan_del() using del_timer() caused l2cap_monitor_timeout()
> be called after the sock was freed, so it raised a system crash.
> So I just replaced del_timer() with del_timer_sync() to solve it.
>
> Signed-off-by: Haijun Liu <haijun.liu@atheros.com>
looks good to me and if Gustavo agrees, then
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: Re[2]: 4.76 possible regression: bluetoothd segfaults when launching bluetooth programs
From: Johan Hedberg @ 2010-10-26 14:19 UTC (permalink / raw)
To: Ilya Basin; +Cc: linux-bluetooth
In-Reply-To: <1602793498.20101026170445@gmail.com>
Hi Ilya,
On Tue, Oct 26, 2010, Ilya Basin wrote:
> JH> have all debug symbols enabled. Could you try to reproduce this with
> JH> latest bluez git. You don't need to install anything but just compile
>
> segfaults start after this commit:
> [d5e700051b1263b2028331d41d60de02a5a6f90e] Fix append_variant_array()
> to take a number of elements
>
> Not every BT program kills bluetoothd, but Smartcam does.
> http://sourceforge.net/projects/smartcam/
> [il@IL bluez]$ smartcam
> smartcam: registered DBUS service "org.gnome.smartcam"
> Found smartcam device file: /dev/video0
> smartcam: started comm thread
> smartcam: port = 1
> sdp_record_register: Protocol error
Thanks for the info. This program seems to add a somehow malformed
service record which is the cause of the crash. Before the patch you
pointed out a NULL pointer was used to detect the end of a pointer array
and so bt_uuid2string() returning NULL for this service record didn't
cause any bad behavior (since the code just stopped iterating a pointer
array after this). However after the patch the code uses an explicit
integer value for the list length and would try to dereference the NULL
pointer in the middle of the list.
I've now pushed a patch to git which should fix this:
http://git.kernel.org/?p=bluetooth/bluez.git;a=commitdiff;h=e31d21c7f238352893a365ab50642707c44087cd
Please do a git pull and see if it really fixes the issue for you.
Thanks.
Johan
^ permalink raw reply
* Re: [PATCH 2/2] Add separate queries for listing size in PBAP
From: Johan Hedberg @ 2010-10-26 14:07 UTC (permalink / raw)
To: Radoslaw Jablonski; +Cc: linux-bluetooth
In-Reply-To: <1288081792-15296-2-git-send-email-ext-jablonski.radoslaw@nokia.com>
Hi Radek,
On Tue, Oct 26, 2010, Radoslaw Jablonski wrote:
> Previosly to get phonebook-size or call history size, full phonebook
> query was used and a lot of unnecessary operations need to be done on
> that returned data. Now separate query is used to get size of each
> listing so performance for "getPhonebookSize" operations improved a lot.
> ---
> plugins/phonebook-tracker.c | 105 ++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 103 insertions(+), 2 deletions(-)
Both patches have been pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH] Add separate queries for listing size in PBAP
From: Radoslaw Jablonski @ 2010-10-26 13:54 UTC (permalink / raw)
To: Radoslaw Jablonski; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1288078932-9547-1-git-send-email-ext-jablonski.radoslaw@nokia.com>
Hi,
On 10/26/2010 10:42 AM, Radoslaw Jablonski wrote:
> "nmo:to ?c ." \
> + "}"
> +
> +
> +#define COMBINED_CALLS_COUNT_QUERY \
> + "SELECT COUNT(?call) WHERE {" \
> + "{" \
>
Please ignore this patch.
I've noticed after sending that has unneeded 2 empty lines instead of
one between these queries.
I resend new version of this patch - so new version of this is the
correct one.
BR,
Radek
^ permalink raw reply
* Re: [RFC] SMP initial draft
From: Anderson Briglia @ 2010-10-26 13:48 UTC (permalink / raw)
To: Ville Tervo; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <20101026093808.GE15050@null>
On 10/26/2010 05:38 AM, Ville Tervo wrote:
> Hi,
>
> On Sat, Oct 23, 2010 at 01:56:54AM +0200, ext Anderson Briglia wrote:
>
>> This RFC is about Security Manager Protocol (SMP).
>> Basically this patchset implements the initial negotiation over L2CAP and LE
>> connections between a Master and a Slave. TK and STK keys are not being generated
>> and/or exchanged yet, do not expect real encryption/decryption by now.
>> Actually, our next tasks are related to integrate current implementation and
>> Criptographic Toolbox ah, c1 and s1 functions for TK and STK key generation for
>> Just Works SMP pairing method.
>>
> I think some kind of mechanism to start encryption on already existing
> connection is needed. It depends on attribute if security is needed or not. One
> might read unsecure attributes and then needs to start encryption because it
> wants to read encrypted attribute.
>
> Is that already planned somehow?
>
Not yet. After we get the SMP keys generation and exchange working we
could start working on it.
>
>> To test this RFC you need to have the Ville Tervo[1] kernel tree with LE connection
>> patches. Of course you need LE devices and bluez git tree (master branch). Just run
>> the bluetoothd and try to list the primary services using gatttool over an LE channel.
>> eg.: gatttool --primary --le -i hci0 -b 00:17:E7:DD:08:FF
>>
> I guess I should take patches which are fixing plain LE (2/6) and (5/6)
> connection to my tree alread. How it sounds to you?
>
I think this is ok. Just update your kernel tree and we could make a
rebase here and separate the "pure" SMP patches on top of your patches.
>
>> Comments are welcome.
>>
>> [1] git://git.kernel.org/pub/scm/linux/kernel/git/vtervo/bluetooth-le-2.6.git
>>
>> [PATCH 1/6] Bluetooth: Add SMP command structures
>> [PATCH 2/6] Bluetooth: fix receiving L2CAP packets over LE
>> [PATCH 3/6] Bluetooth: Implement the first SMP commands
>> [PATCH 4/6] Bluetooth: Start SMP procedure
>> [PATCH 5/6] Bluetooth: Fix initiated LE connections
>> [PATCH 6/6] Bluetooth: simple SMP pairing negotiation
>> --
>> 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
>>
^ permalink raw reply
* Re[2]: 4.76 possible regression: bluetoothd segfaults when launching bluetooth programs
From: Ilya Basin @ 2010-10-26 13:04 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <20101025204015.GA19748@jh-x301>
[-- Attachment #1: Type: text/plain, Size: 603 bytes --]
JH> have all debug symbols enabled. Could you try to reproduce this with
JH> latest bluez git. You don't need to install anything but just compile
segfaults start after this commit:
[d5e700051b1263b2028331d41d60de02a5a6f90e] Fix append_variant_array()
to take a number of elements
Not every BT program kills bluetoothd, but Smartcam does.
http://sourceforge.net/projects/smartcam/
[il@IL bluez]$ smartcam
smartcam: registered DBUS service "org.gnome.smartcam"
Found smartcam device file: /dev/video0
smartcam: started comm thread
smartcam: port = 1
sdp_record_register: Protocol error
--
[-- Attachment #2: gdb-new.txt --]
[-- Type: TEXT/PLAIN, Size: 20150 bytes --]
[root@IL bluez]# gdb --args ./src/.libs/bluetoothd -nd
GNU gdb (GDB) 7.2
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law. Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /.snapshots/persist/builds/bluez-git/bluez/src/.libs/bluetoothd...done.
(gdb) r
Starting program: /.snapshots/persist/builds/bluez-git/bluez/src/.libs/bluetoothd -nd
[Thread debugging using libthread_db enabled]
bluetoothd[5329]: Bluetooth deamon 4.76
bluetoothd[5329]: src/main.c:parse_config() parsing main.conf
bluetoothd[5329]: src/main.c:parse_config() discovto=0
bluetoothd[5329]: src/main.c:parse_config() pairto=0
bluetoothd[5329]: src/main.c:parse_config() pageto=8192
bluetoothd[5329]: src/main.c:parse_config() name=%h-%d
bluetoothd[5329]: src/main.c:parse_config() class=0x000100
bluetoothd[5329]: src/main.c:parse_config() discov_interval=0
bluetoothd[5329]: src/main.c:parse_config() Key file does not have key 'DeviceID'
bluetoothd[5329]: Starting SDP server
bluetoothd[5329]: src/plugin.c:plugin_init() Loading builtin plugins
bluetoothd[5329]: src/plugin.c:add_plugin() Loading audio plugin
bluetoothd[5329]: src/plugin.c:add_plugin() Loading input plugin
bluetoothd[5329]: src/plugin.c:add_plugin() Loading serial plugin
bluetoothd[5329]: src/plugin.c:add_plugin() Loading network plugin
bluetoothd[5329]: src/plugin.c:add_plugin() Loading service plugin
bluetoothd[5329]: src/plugin.c:add_plugin() Loading hciops plugin
bluetoothd[5329]: src/plugin.c:add_plugin() Loading formfactor plugin
bluetoothd[5329]: src/plugin.c:add_plugin() Loading storage plugin
bluetoothd[5329]: src/plugin.c:plugin_init() Loading plugins /usr/lib/bluetooth/plugins
bluetoothd[5329]: Version mismatch for netlink
bluetoothd[5329]: plugins/service.c:register_interface() path /org/bluez/5329/any
bluetoothd[5329]: plugins/service.c:register_interface() Registered interface org.bluez.Service on path /org/bluez/5329/any
bluetoothd[5329]: network/manager.c:read_config() /etc/bluetooth/network.conf: Key file does not have key 'DisableSecurity'
bluetoothd[5329]: network/manager.c:read_config() Config options: Security=true
bluetoothd[5329]: input/manager.c:input_manager_init() input.conf: Key file does not have key 'IdleTimeout'
bluetoothd[5329]: audio/manager.c:audio_manager_init() audio.conf: Key file does not have key 'AutoConnect'
bluetoothd[5329]: audio/unix.c:unix_init() Unix socket created: 10
bluetoothd[5329]: audio/headset.c:telephony_ready_ind() Telephony plugin initialized
bluetoothd[5329]: audio/headset.c:print_ag_features() HFP AG features: "Ability to reject a call" "Enhanced call status" "Extended Error Result Codes"
bluetoothd[5329]: HCI dev 0 registered
bluetoothd[5329]: plugins/hciops.c:init_device() child 5332 forked
bluetoothd[5329]: src/adapter.c:btd_adapter_ref() 0xb800d600: ref=1
bluetoothd[5329]: HCI dev 0 up
bluetoothd[5329]: Starting security manager 0
bluetoothd[5329]: src/adapter.c:btd_adapter_set_class() Changing Major/Minor class to 0x000100
bluetoothd[5329]: src/adapter.c:adapter_start() Stopping Inquiry at adapter startup
bluetoothd[5329]: plugins/service.c:register_interface() path /org/bluez/5329/hci0
bluetoothd[5329]: plugins/service.c:register_interface() Registered interface org.bluez.Service on path /org/bluez/5329/hci0
bluetoothd[5329]: network/manager.c:network_server_probe() path /org/bluez/5329/hci0
bluetoothd[5329]: src/adapter.c:btd_adapter_ref() 0xb800d600: ref=2
bluetoothd[5329]: network/server.c:server_register() Registered interface org.bluez.NetworkServer on path /org/bluez/5329/hci0
bluetoothd[5329]: serial/manager.c:proxy_probe() path /org/bluez/5329/hci0
bluetoothd[5329]: src/adapter.c:btd_adapter_ref() 0xb800d600: ref=3
bluetoothd[5329]: serial/proxy.c:proxy_register() Registered interface org.bluez.SerialProxyManager on path /org/bluez/5329/hci0
bluetoothd[5329]: src/adapter.c:btd_adapter_ref() 0xb800d600: ref=4
bluetoothd[5329]: audio/manager.c:headset_server_probe() path /org/bluez/5329/hci0
bluetoothd[5329]: src/adapter.c:btd_adapter_ref() 0xb800d600: ref=5
bluetoothd[5329]: audio/manager.c:audio_adapter_ref() 0xb800dd80: ref=1
bluetoothd[5329]: audio/manager.c:headset_server_init() audio.conf: Key file does not have key 'Master'
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Adding record with handle 0x10000
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00000003-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00000100-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00001002-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00001108-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00001112-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00001203-0000-1000-8000-00805f9
bluetoothd[5329]: audio/headset.c:headset_config_init() audio.conf: Key file does not have key 'SCORouting'
bluetoothd[5329]: audio/headset.c:headset_config_init() audio.conf: Key file does not have key 'FastConnectable'
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Adding record with handle 0x10001
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00000003-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00000100-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00001002-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 0000111e-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 0000111f-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00001203-0000-1000-8000-00805f9
bluetoothd[5329]: audio/manager.c:a2dp_server_probe() path /org/bluez/5329/hci0
bluetoothd[5329]: audio/manager.c:audio_adapter_ref() 0xb800dd80: ref=2
bluetoothd[5329]: audio/a2dp.c:a2dp_register() audio.conf: Key file does not have key 'Enable'
bluetoothd[5329]: audio/a2dp.c:a2dp_register() audio.conf: Key file does not have key 'Disable'
bluetoothd[5329]: audio/a2dp.c:a2dp_register() audio.conf: Key file does not have group 'A2DP'
bluetoothd[5329]: audio/a2dp.c:a2dp_register() audio.conf: Key file does not have group 'A2DP'
bluetoothd[5329]: audio/a2dp.c:a2dp_register() audio.conf: Key file does not have group 'A2DP'
bluetoothd[5329]: audio/a2dp.c:a2dp_register() audio.conf: Key file does not have group 'A2DP'
bluetoothd[5329]: audio/avdtp.c:avdtp_init() audio.conf: Key file does not have key 'Master'
bluetoothd[5329]: audio/avdtp.c:avdtp_register_sep() SEP 0xb800e730 registered: type:0 codec:0 seid:1
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Adding record with handle 0x10002
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00000019-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00000100-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00001002-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 0000110a-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 0000110d-0000-1000-8000-00805f9
bluetoothd[5329]: audio/manager.c:avrcp_server_probe() path /org/bluez/5329/hci0
bluetoothd[5329]: audio/manager.c:audio_adapter_ref() 0xb800dd80: ref=3
bluetoothd[5329]: audio/control.c:avrcp_register() audio.conf: Key file does not have key 'Master'
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Adding record with handle 0x10003
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00000017-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00000100-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00001002-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 0000110c-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 0000110e-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Adding record with handle 0x10004
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00000017-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00000100-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 00001002-0000-1000-8000-00805f9
bluetoothd[5329]: src/sdpd-service.c:add_record_to_server() Record pattern UUID 0000110e-0000-1000-8000-00805f9
bluetoothd[5329]: plugins/formfactor.c:formfactor_probe() Setting 0x000100 for major/minor device class
bluetoothd[5329]: Clearing blocked list failed: Invalid argument (22)
bluetoothd[5329]: src/device.c:device_create() Creating device /org/bluez/5329/hci0/dev_00_1B_98_A3_A5_2B
bluetoothd[5329]: src/device.c:btd_device_ref() 0xb80299e0: ref=1
bluetoothd[5329]: src/device.c:device_probe_drivers() Probe drivers for /org/bluez/5329/hci0/dev_00_1B_98_A3_A5_2B
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_00_1B_98_A3_A5_2B: 00001101-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/port.c:create_serial_device() Registered interface org.bluez.Serial on path /org/bluez/5329/hci0/dev_00_1B_98_A3_A5_2B
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_00_1B_98_A3_A5_2B: 00001103-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_00_1B_98_A3_A5_2B: 00001105-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_00_1B_98_A3_A5_2B: 00001106-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_00_1B_98_A3_A5_2B: 00001112-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_00_1B_98_A3_A5_2B: 0000111f-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_00_1B_98_A3_A5_2B: db1d8f12-95f3-402c-9b97-bc504c9a55c4
bluetoothd[5329]: input/manager.c:headset_probe() path /org/bluez/5329/hci0/dev_00_1B_98_A3_A5_2B
bluetoothd[5329]: probe failed with driver input-headset for device /org/bluez/5329/hci0/dev_00_1B_98_A3_A5_2B
bluetoothd[5329]: src/adapter.c:adapter_get_device() 00:1B:98:A3:A5:2B
bluetoothd[5329]: src/device.c:btd_device_ref() 0xb80299e0: ref=2
bluetoothd[5329]: audio/device.c:audio_device_register() Registered interface org.bluez.Audio on path /org/bluez/5329/hci0/dev_00_1B_98_A3_A5_2B
bluetoothd[5329]: audio/manager.c:handle_uuid() server not enabled for 00001112-0000-1000-8000-00805f9b34fb (0x1112)
bluetoothd[5329]: audio/manager.c:handle_uuid() server not enabled for 0000111f-0000-1000-8000-00805f9b34fb (0x111f)
bluetoothd[5329]: audio/manager.c:handle_uuid() server not enabled for 0000110a-0000-1000-8000-00805f9b34fb (0x110a)
bluetoothd[5329]: audio/manager.c:handle_uuid() Found AV Target
bluetoothd[5329]: audio/control.c:control_init() Registered interface org.bluez.Control on path /org/bluez/5329/hci0/dev_00_1B_98_A3_A5_2B
bluetoothd[5329]: audio/manager.c:handle_uuid() Found AV Target
bluetoothd[5329]: src/device.c:device_create() Creating device /org/bluez/5329/hci0/dev_00_1D_6E_4F_54_EA
bluetoothd[5329]: src/device.c:btd_device_ref() 0xb802cf08: ref=1
bluetoothd[5329]: src/device.c:device_probe_drivers() Probe drivers for /org/bluez/5329/hci0/dev_00_1D_6E_4F_54_EA
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_00_1D_6E_4F_54_EA: 00000002-0000-1000-8000-0002ee000002
bluetoothd[5329]: serial/port.c:create_serial_device() Registered interface org.bluez.Serial on path /org/bluez/5329/hci0/dev_00_1D_6E_4F_54_EA
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_00_1D_6E_4F_54_EA: 00001103-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_00_1D_6E_4F_54_EA: 00001105-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_00_1D_6E_4F_54_EA: 00001106-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_00_1D_6E_4F_54_EA: 00001112-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_00_1D_6E_4F_54_EA: 0000111b-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_00_1D_6E_4F_54_EA: 0000111f-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_00_1D_6E_4F_54_EA: 00005005-0000-1000-8000-0002ee000001
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_00_1D_6E_4F_54_EA: 00005601-0000-1000-8000-0002ee000001
bluetoothd[5329]: input/manager.c:headset_probe() path /org/bluez/5329/hci0/dev_00_1D_6E_4F_54_EA
bluetoothd[5329]: probe failed with driver input-headset for device /org/bluez/5329/hci0/dev_00_1D_6E_4F_54_EA
bluetoothd[5329]: src/adapter.c:adapter_get_device() 00:1D:6E:4F:54:EA
bluetoothd[5329]: src/device.c:btd_device_ref() 0xb802cf08: ref=2
bluetoothd[5329]: audio/device.c:audio_device_register() Registered interface org.bluez.Audio on path /org/bluez/5329/hci0/dev_00_1D_6E_4F_54_EA
bluetoothd[5329]: audio/manager.c:handle_uuid() server not enabled for 00001112-0000-1000-8000-00805f9b34fb (0x1112)
bluetoothd[5329]: audio/manager.c:handle_uuid() server not enabled for 0000111f-0000-1000-8000-00805f9b34fb (0x111f)
bluetoothd[5329]: audio/manager.c:handle_uuid() server not enabled for 0000110a-0000-1000-8000-00805f9b34fb (0x110a)
bluetoothd[5329]: audio/manager.c:handle_uuid() Found AV Target
bluetoothd[5329]: audio/control.c:control_init() Registered interface org.bluez.Control on path /org/bluez/5329/hci0/dev_00_1D_6E_4F_54_EA
bluetoothd[5329]: audio/manager.c:handle_uuid() Found AV Target
bluetoothd[5329]: src/device.c:device_create() Creating device /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB
bluetoothd[5329]: src/device.c:btd_device_ref() 0xb8031df0: ref=1
bluetoothd[5329]: src/device.c:device_probe_drivers() Probe drivers for /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB: 00000002-0000-1000-8000-0002ee000002
bluetoothd[5329]: serial/port.c:create_serial_device() Registered interface org.bluez.Serial on path /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB: 00000004-0000-1000-8000-0002ee000002
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB: 00001103-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB: 00001105-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB: 00001106-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB: 00001112-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB: 0000111b-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB: 0000111f-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB: 0000112f-0000-1000-8000-00805f9b34fb
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB: 00005005-0000-1000-8000-0002ee000001
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB: 00005557-0000-1000-8000-0002ee000001
bluetoothd[5329]: serial/manager.c:serial_probe() path /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB: 00005601-0000-1000-8000-0002ee000001
bluetoothd[5329]: input/manager.c:headset_probe() path /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB
bluetoothd[5329]: probe failed with driver input-headset for device /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB
bluetoothd[5329]: src/adapter.c:adapter_get_device() A8:7E:33:D7:29:DB
bluetoothd[5329]: src/device.c:btd_device_ref() 0xb8031df0: ref=2
bluetoothd[5329]: audio/device.c:audio_device_register() Registered interface org.bluez.Audio on path /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB
bluetoothd[5329]: audio/manager.c:handle_uuid() server not enabled for 00001112-0000-1000-8000-00805f9b34fb (0x1112)
bluetoothd[5329]: audio/manager.c:handle_uuid() server not enabled for 0000111f-0000-1000-8000-00805f9b34fb (0x111f)
bluetoothd[5329]: audio/manager.c:handle_uuid() server not enabled for 0000110a-0000-1000-8000-00805f9b34fb (0x110a)
bluetoothd[5329]: audio/manager.c:handle_uuid() Found AV Target
bluetoothd[5329]: audio/control.c:control_init() Registered interface org.bluez.Control on path /org/bluez/5329/hci0/dev_A8_7E_33_D7_29_DB
bluetoothd[5329]: audio/manager.c:handle_uuid() Found AV Remote
bluetoothd[5329]: Adapter /org/bluez/5329/hci0 has been enabled
bluetoothd[5329]: src/main.c:main() Entering main loop
bluetoothd[5329]: plugins/hciops.c:child_exit() child 5332 exited
bluetoothd[5329]: src/rfkill.c:rfkill_event() RFKILL event idx 0 type 2 op 0 soft 0 hard 0
bluetoothd[5329]: src/rfkill.c:rfkill_event() RFKILL event idx 1 type 1 op 0 soft 0 hard 0
bluetoothd[5329]: Inquiry Failed with status 0x12
Program received signal SIGSEGV, Segmentation fault.
0xb7d2a653 in strlen () from /lib/libc.so.6
(gdb) bt
#0 0xb7d2a653 in strlen () from /lib/libc.so.6
#1 0xb7e53b10 in ?? () from /usr/lib/libdbus-1.so.3
#2 0xb7e3f34b in ?? () from /usr/lib/libdbus-1.so.3
#3 0xb7e437a9 in dbus_message_iter_append_basic () from /usr/lib/libdbus-1.so.3
#4 0xb7feebb4 in append_array_variant (iter=0xbffff584, type=115, val=0xbffff5f0, n_elements=6) at src/dbus-common.c:228
#5 0xb7feee58 in emit_array_property_changed (conn=0xb8009eb8, path=0xb8015860 "/org/bluez/5329/hci0", interface=0xb7ffd4b8 "org.bluez.Adapter",
name=0xb7ffd638 "UUIDs", type=115, value=0xbffff5f0, num=6) at src/dbus-common.c:320
#6 0xb7fe400e in adapter_emit_uuids_updated (adapter=0xb800d600) at src/adapter.c:1107
#7 0xb7fe40fb in adapter_service_ins_rem (bdaddr=0xbffff770, rec=0xb800d5d8, insert=1) at src/adapter.c:1146
#8 0xb7fe4133 in adapter_service_insert (bdaddr=0xbffff770, rec=0xb800d5d8) at src/adapter.c:1153
#9 0xb7fd596a in sdp_record_add (device=0xbffff770, rec=0xb800d5d8) at src/sdpd-database.c:188
#10 0xb7fd520e in service_register_req (req=0xbffff770, rsp=0xbffff70c) at src/sdpd-service.c:684
#11 0xb7fd3ab2 in process_request (req=0xbffff770) at src/sdpd-request.c:992
#12 0xb7fd3de1 in handle_request (sk=25, data=0xb8029890 "u", len=119) at src/sdpd-request.c:1087
#13 0xb7fd1e01 in io_session_event (chan=0xb800cda0, cond=G_IO_IN, data=0xb8004990) at src/sdpd-server.c:188
#14 0xb7eeca2b in ?? () from /usr/lib/libglib-2.0.so.0
#15 0xb7ea5b72 in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#16 0xb7ea6350 in ?? () from /usr/lib/libglib-2.0.so.0
#17 0xb7ea6a1b in g_main_loop_run () from /usr/lib/libglib-2.0.so.0
#18 0xb7fce93c in main (argc=1, argv=0xbffffad4) at src/main.c:486
(gdb)
^ permalink raw reply
* Re: [PATCH 1/9] mfd: Add support for the ST-Ericsson CG2900.
From: Arnd Bergmann @ 2010-10-26 12:06 UTC (permalink / raw)
To: Par-Gunnar Hjalmdahl; +Cc: linus.walleij, linux-bluetooth, linux-kernel
In-Reply-To: <AANLkTinY0qmt6uFo4PZ3xH2BF5-AJ0C7ptctzNdOkRU2@mail.gmail.com>
On Tuesday 26 October 2010, Par-Gunnar Hjalmdahl wrote:
> 2010/10/22 Arnd Bergmann <arnd@arndb.de>:
> > On Friday 22 October 2010 12:35:16 Par-Gunnar Hjalmdahl wrote:
> >> This patch adds support for the ST-Ericsson CG2900 Connectivity
> >> Combo controller.
> >> This patch adds the central framework to be able to register CG2900 users,
> >> transports, and chip handlers.
> >>
> >> Signed-off-by: Par-Gunnar Hjalmdahl <par-gunnar.p.hjalmdahl@stericsson.com>
> >
> > Looks ok from a coding style perspective, but some important information is
> > missing from the description:
> >
> > * What is a CG2900?
> > * Why is it a MFD device rather than e.g. a bus or a subsystem?
> >
>
> I will add some more info:
> - CG2900 is a GPS, Bluetooth, FM controller
> - I do not really know what makes a device qualify as a certain type,
> but at least for us this is a multifunction device. But I can't really
> say that it isn't really a bus (could be stated as multifunction HCI
> bus). I guess it could be qualified as a subsystem as well, but I
> cannot give you a reason to have it as either.
It's not often completely clear, but I would draw the distinction like
this:
* A multi-function device is a single device that sits on a given bus
with one host I/O interface and provides functionality to different
logical subsystems like serial or alsa. A typical case for an MFD would
be a to solve the problem of sharing resources between the child drivers
because you cannot bind one device to two drivers. If CG2900 is a single
piece of silicon that always looks the same way, it's probably an MFD.
* A bus is a much more general abstraction which has multiple devices
and multiple device types behind it. The bus has no idea what devices
are attached to it at compile time, so you either probe the devices at
boot time or define a set of devices in a board definition (platform
devices). Driver modules then register a struct device_driver for the
bus, which gives all matching devices to the bus. If you can have
future CG2900 compatible devices that need to have other drivers, e.g.
for HID or video, it should probably be a bus.
* A subsystem is less clearly defined, I would call bluetooth a subsystem
instead of just a bus because it not only matches devices with drivers
but also has its own user-level interfaces for raw communication.
If you want to be able to have drivers in the kernel and in user space,
you might want to consider starting a new drivers/cg2900 subsystem, or
integrating into the bluetooth subsystem if that makes sense.
> >> +config MFD_CG2900
> >> + tristate "Support ST-Ericsson CG2900 main structure"
> >> + depends on NET
> >> + help
> >> + Support for ST-Ericsson CG2900 Connectivity Combo controller main structure.
> >> + Supports multiple functionalities muxed over a Bluetooth HCI H:4 interface.
> >> + CG2900 support Bluetooth, FM radio, and GPS.
> >> +
> >
> > Can you explain what it means to mux over a H:4 interface? Does this mean
> > you use bluetooth infrastructure that is designed for wireless communication
> > in order to connect on-board or on-chip devices?
> >
>
> The H:4 protocol is defined in the Bluetooth Core specification. It
> uses a single byte first in the data packet to determine an HCI
> channel. In the Bluetooth Core specification there are 4 channels
> specified, 0x01-0x04 (0x01 is BT command, etc). The rest of the
> channels are reserved, but these have been used on a proprietary basis
> to transport data to different IPs within the controller. In our API
> we have chosen to have a channel separation on an API basis and the
> H:4 byte is then added within the driver. So we have multiple channels
> coming in from the users that we mux onto a single data transport.
> That's what I mean in the text. I guess I could rewrite it a bit to
> make it clearer.
I still don't understand it, though that may be a problem on my side.
Does that mean that cg2900 is to the host side a standard bluetooth HCI,
but it uses extensions to the HCI in order to make the other functions
available to the host?
What chapter in the bluetooth core specification describes this?
> >> +
> >> +/**
> >> + * add_h4_user() - Add H4 user to user storage based on supplied H4 channel.
> >> + * @dev: Stored CG2900 device.
> >> + * @name: Device name to identify different devices that are using
> >> + * the same H4 channel.
> >> + *
> >> + * Returns:
> >> + * 0 if there is no error.
> >> + * -EINVAL if NULL pointer or bad channel is supplied.
> >> + * -EBUSY if there already is a user for supplied channel.
> >> + */
> >> +static int add_h4_user(struct cg2900_device *dev, const char * const name)
> >> +{
> >
> > Where does that name come from? Why not just use an enum if the name is
> > only use for strncmp?
> >
>
> At a point in time we used to have an enum, but from what I can see it
> is easier to keep an API stable if you use name lookup instead. If we
> want to have minimal changes to the API in the future this is quite a
> flexible solution, but this code should be moved to each respective
> chip handler.
You haven't really answered the first question, where the name comes from
(I guess the question was not too clear). Does this e.g. get passed by a
user application, or does it come from a hardware description of some
sort?
It looks a bit like a handcoded version of the code we use for device/driver
matching in drivers/core. If that's the case, it would be better to use
the existing infrastructure and create a bus with devices that can be
matched with drivers.
> >> +static ssize_t test_char_dev_read(struct file *filp, char __user *buf,
> >> + size_t count, loff_t *f_pos)
> >> +{
> >> + struct sk_buff *skb;
> >> + int bytes_to_copy;
> >> + int err;
> >> + struct sk_buff_head *rx_queue = &core_info->test_char_dev->rx_queue;
> >
> > What is this read/write interface used for?
> >
> > The name suggests that this is only for testing and not an essential
> > part of your driver. Could this be made a separate driver?
> >
> > It also looks like you do socket communication here, can't you use
> > a proper AF_BLUETOOTH socket to do the same?
> >
>
> This interface can be used for module testing where you can have a
> test engine in user space that acts as a chip. Yes, it could be made
> into a separate driver but it would be a very small driver. Don't know
> if it will be worth having it in a separate file...
I don't think file size is a major argument here. If it's used for
testing, I'd make it a separate module that defaults to not being
built, in order to prevent people from relying on what you intended
as a testing interface.
> We use the sk_buffer, which I guess is the reason that you mention
> sockets. We could of course use a socket instead of a char dev, both
> here and in the cg2900_char_dev file (which of course then would be
> renamed). It was just a choice we made during development. We think
> that the transport as such is more like a streaming interface, but I
> see no real problem to have sockets. We already have several users
> using char dev though so I would prefer to keep char dev rather than
> switching to sockets unless you have a strong reason for this.
> I see no reason to use AF_BLUETOOTH though, the transport is not only
> for Bluetooth.
The question here is what is appropriate. My guess was that the frame
format was the one used in bluetooth sockets. If that's not the
case, AF_BLUETOOTH would obviously be wrong.
For the test interface, it doesn't matter that much what you use, but
for the cg2900_char_dev we should make sure that you considered all
the options and made a reasonable decision. What does the data format
look like there? My impression is that the character device might be
too low-level and it could be better to use some structured interface
like a socket, but it really depends on what it actually does.
In particular, if you can address multiple hardware units over one
character device, it may be better to have separate devices for each
of them.
> >> + CG2900_INFO("test_char_dev_read");
> >> +
> >> + if (skb_queue_empty(rx_queue))
> >> + wait_event_interruptible(char_wait_queue,
> >> + !(skb_queue_empty(rx_queue)));
> >> +
> >> + skb = skb_dequeue(rx_queue);
> >> + if (!skb) {
> >> + CG2900_INFO("skb queue is empty - return with zero bytes");
> >> + bytes_to_copy = 0;
> >> + goto finished;
> >> + }
> >> +
> >> + bytes_to_copy = min(count, skb->len);
> >> + err = copy_to_user(buf, skb->data, bytes_to_copy);
> >> + if (err) {
> >> + skb_queue_head(rx_queue, skb);
> >> + return -EFAULT;
> >> + }
> >> +
> >> + skb_pull(skb, bytes_to_copy);
> >> +
> >> + if (skb->len > 0)
> >> + skb_queue_head(rx_queue, skb);
> >> + else
> >> + kfree_skb(skb);
> >> +
> >> +finished:
> >> + return bytes_to_copy;
> >> +}
> >
> > This does not handle short/continued reads.
> >
>
> As I mentioned above this interface is used for testing and we
> therefore have some restriction of usage. I don't think we need to
> impose all things necessary for a full interface upon it.
This is where it gets complicated. We don't normally add interfaces
to the kernel unless we expect to fully support them for the forseeable
future. It has happened more than enough in the past that somebody
introduced a debugging interface which subsequently became a supported
ABI because some application started relying on it.
Is the code that uses the test interface even publically available?
If not, I would recommend saving the trouble of arguing about it
and leaving out the interface.
> >> + #define CG2900_ERR(fmt, arg...) \
> >> + do { \
> >> + if (cg2900_debug_level >= 1) \
> >> + pr_err("CG2900 %s: " fmt "\n" , __func__ , ## arg); \
> >> + } while (0)
> >> +
> >> +#endif /* NDEBUG */
> >
> > You'll feel relieved when all this is gone...
> >
>
> The only thing is it's been pretty nice to have it, but I will remove it.
> Is it OK to keep defines so we can have "CG2900" in front and "\n"
> after the text?
Ideally, you would use the dev_* functions instead of the pr_* functions,
which print the name of the device before the message. I also wouldn't
add the "\n" in the macro because all the regular printing functions don't
do it. It will likely only confuse people and the binary remains the same.
Arnd
^ permalink raw reply
* Re: [PATCH 2/2 v2] Bluetooth: Fix system crash bug of no send queue protect
From: haijun liu @ 2010-10-26 11:50 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: Haijun Liu, linux-bluetooth
In-Reply-To: <20101025110908.GB7721@vigoh>
Hi Gustavo,
> sk->sk_send_head is also protected by the socket lock.
>
I thought you mean sk->tx_queue.lock, actually it's sk->sk_lock.slock.
But there could be some paths have not been covered.
> This dump shows that the crash happens for a code that is not mainline
> yet. I can't take a patch that fix a bug for code not in mainline. You
> have to show the bug using mainline code.
>
Yes, it is not mainline code, but this issue could be common issue.
OK, let me try use sk->sk_lock.slock to solve this bug.
--
Haijun Liu
^ permalink raw reply
* Re: [RFC] SMP initial draft
From: Ville Tervo @ 2010-10-26 9:38 UTC (permalink / raw)
To: ext Anderson Briglia; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <1287791820-22693-1-git-send-email-anderson.briglia@openbossa.org>
Hi,
On Sat, Oct 23, 2010 at 01:56:54AM +0200, ext Anderson Briglia wrote:
> This RFC is about Security Manager Protocol (SMP).
> Basically this patchset implements the initial negotiation over L2CAP and LE
> connections between a Master and a Slave. TK and STK keys are not being generated
> and/or exchanged yet, do not expect real encryption/decryption by now.
> Actually, our next tasks are related to integrate current implementation and
> Criptographic Toolbox ah, c1 and s1 functions for TK and STK key generation for
> Just Works SMP pairing method.
I think some kind of mechanism to start encryption on already existing
connection is needed. It depends on attribute if security is needed or not. One
might read unsecure attributes and then needs to start encryption because it
wants to read encrypted attribute.
Is that already planned somehow?
>
> To test this RFC you need to have the Ville Tervo[1] kernel tree with LE connection
> patches. Of course you need LE devices and bluez git tree (master branch). Just run
> the bluetoothd and try to list the primary services using gatttool over an LE channel.
> eg.: gatttool --primary --le -i hci0 -b 00:17:E7:DD:08:FF
I guess I should take patches which are fixing plain LE (2/6) and (5/6)
connection to my tree alread. How it sounds to you?
> Comments are welcome.
>
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/vtervo/bluetooth-le-2.6.git
>
> [PATCH 1/6] Bluetooth: Add SMP command structures
> [PATCH 2/6] Bluetooth: fix receiving L2CAP packets over LE
> [PATCH 3/6] Bluetooth: Implement the first SMP commands
> [PATCH 4/6] Bluetooth: Start SMP procedure
> [PATCH 5/6] Bluetooth: Fix initiated LE connections
> [PATCH 6/6] Bluetooth: simple SMP pairing negotiation
> --
> 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
^ permalink raw reply
* Re: [PATCH 5/6] Bluetooth: Fix initiated LE connections
From: Ville Tervo @ 2010-10-26 9:28 UTC (permalink / raw)
To: ext Anderson Briglia
Cc: linux-bluetooth@vger.kernel.org, Vinicius Costa Gomes
In-Reply-To: <1287791820-22693-6-git-send-email-anderson.briglia@openbossa.org>
Hi,
On Sat, Oct 23, 2010 at 01:56:59AM +0200, ext Anderson Briglia wrote:
> From: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
>
> Fix LE connections not being marked as master.
>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
> ---
> net/bluetooth/hci_conn.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index c7309e4..1a48c6a 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -52,6 +52,7 @@ void hci_le_connect(struct hci_conn *conn)
>
> conn->state = BT_CONNECT;
> conn->out = 1;
> + conn->link_mode |= HCI_LM_MASTER;
conn->out and conn->link_mode are redutant information. The role cannot be changed.
>
> memset(&cp, 0, sizeof(cp));
> cp.scan_interval = cpu_to_le16(0x0004);
> --
> 1.7.0.4
--
Ville
^ permalink raw reply
* Re: [PATCH 3/6] Bluetooth: Implement the first SMP commands
From: Ville Tervo @ 2010-10-26 9:26 UTC (permalink / raw)
To: ext Gustavo F. Padovan
Cc: Anderson Briglia, linux-bluetooth@vger.kernel.org,
Vinicius Costa Gomes
In-Reply-To: <20101025130356.GA8862@vigoh>
On Mon, Oct 25, 2010 at 03:03:56PM +0200, ext Gustavo F. Padovan wrote:
> Hi Vinicius,
>
> * Anderson Briglia <anderson.briglia@openbossa.org> [2010-10-22 19:56:57 -0400]:
>
> > From: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
> >
> > These simple commands will allow the SMP procedure to be started
> > and terminated with a not supported error. This is the first step
> > toward something useful.
> >
> > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
> > ---
> > net/bluetooth/l2cap.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 files changed, 117 insertions(+), 0 deletions(-)
> >
> > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > index 1ac44f4..ba87c84 100644
> > --- a/net/bluetooth/l2cap.c
> > +++ b/net/bluetooth/l2cap.c
> > @@ -54,6 +54,7 @@
> > #include <net/bluetooth/bluetooth.h>
> > #include <net/bluetooth/hci_core.h>
> > #include <net/bluetooth/l2cap.h>
> > +#include <net/bluetooth/smp.h>
> >
> > #define VERSION "2.15"
> >
> > @@ -307,6 +308,85 @@ static void l2cap_chan_del(struct sock *sk, int err)
> > }
> > }
> >
> > +static struct sk_buff *smp_build_cmd(struct l2cap_conn *conn, u8 code,
> > + u16 dlen, void *data)
>
> Call this l2cap_smp_build_cmd()
Should the whole smp stuff be in separate file (smp.c)? It's not a l2cap feature but a
protocol using l2cap. In that case smp_build_cmd would be good name.
--
Ville
^ permalink raw reply
* [PATCH] Fix cache is invalid on empty listing response
From: Dmitriy Paliy @ 2010-10-26 8:33 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Dmitriy Paliy
Fix cache marked as invalid when no data available in response to get phone
book listing. This fixes obexd crash in 3-way calling scenario. Cache is
created when there is second incoming call. If the call is rejected and
retried again, valid empty cache is sent over obex stream that causes obexd
crash. With this fix, the stream is aborted in such scenario. A new attempt
to create cache is carried out on each new subsequent incoming call.
---
plugins/pbap.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/plugins/pbap.c b/plugins/pbap.c
index 11cb678..f7f3897 100644
--- a/plugins/pbap.c
+++ b/plugins/pbap.c
@@ -407,7 +407,11 @@ static void cache_ready_notify(void *user_data)
(const char *) pbap->params->searchval);
if (sorted == NULL) {
- pbap->cache.valid = TRUE;
+ /* When no data available from tracker, cache marked as
+ * invalid (new one will be tried to be created next time),
+ * no such file or directory error is set, and obex stream
+ * is aborted */
+ pbap->cache.valid = FALSE;
obex_object_set_io_flags(pbap, G_IO_ERR, -ENOENT);
return;
}
--
1.7.0.4
^ permalink raw reply related
* [PATCH 2/2] Add separate queries for listing size in PBAP
From: Radoslaw Jablonski @ 2010-10-26 8:29 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Radoslaw Jablonski
In-Reply-To: <1288081792-15296-1-git-send-email-ext-jablonski.radoslaw@nokia.com>
Previosly to get phonebook-size or call history size, full phonebook
query was used and a lot of unnecessary operations need to be done on
that returned data. Now separate query is used to get size of each
listing so performance for "getPhonebookSize" operations improved a lot.
---
plugins/phonebook-tracker.c | 105 ++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 103 insertions(+), 2 deletions(-)
diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
index 277290c..3367e49 100644
--- a/plugins/phonebook-tracker.c
+++ b/plugins/phonebook-tracker.c
@@ -45,6 +45,7 @@
#define TRACKER_DEFAULT_CONTACT_ME "http://www.semanticdesktop.org/ontologies/2007/03/22/nco#default-contact-me"
#define CONTACTS_ID_COL 38
#define PULL_QUERY_COL_AMOUNT 39
+#define COUNT_QUERY_COL_AMOUNT 1
#define COL_HOME_NUMBER 0
#define COL_HOME_EMAIL 7
#define COL_WORK_NUMBER 8
@@ -579,6 +580,59 @@
"<%s> nco:hasPhoneNumber ?t . " \
"} "
+#define CONTACTS_COUNT_QUERY \
+ "SELECT COUNT(?c) " \
+ "WHERE {" \
+ "?c a nco:PersonContact ." \
+ "FILTER (regex(str(?c), \"contact:\") || " \
+ "regex(str(?c), \"nco#default-contact-me\"))" \
+ "}"
+
+#define MISSED_CALLS_COUNT_QUERY \
+ "SELECT COUNT(?call) WHERE {" \
+ "?c a nco:Contact ;" \
+ "nco:hasPhoneNumber ?h ." \
+ "?call a nmo:Call ;" \
+ "nmo:isSent false ;" \
+ "nmo:from ?c ;" \
+ "nmo:isAnswered false ." \
+ "}"
+
+#define INCOMING_CALLS_COUNT_QUERY \
+ "SELECT COUNT(?call) WHERE {" \
+ "?c a nco:Contact ;" \
+ "nco:hasPhoneNumber ?h ." \
+ "?call a nmo:Call ;" \
+ "nmo:isSent false ;" \
+ "nmo:from ?c ;" \
+ "nmo:isAnswered true ." \
+ "}"
+
+#define OUTGOING_CALLS_COUNT_QUERY \
+ "SELECT COUNT(?call) WHERE {" \
+ "?c a nco:Contact ;" \
+ "nco:hasPhoneNumber ?h ." \
+ "?call a nmo:Call ;" \
+ "nmo:isSent true ;" \
+ "nmo:to ?c ." \
+ "}"
+
+#define COMBINED_CALLS_COUNT_QUERY \
+ "SELECT COUNT(?call) WHERE {" \
+ "{" \
+ "?c a nco:Contact ;" \
+ "nco:hasPhoneNumber ?h ." \
+ "?call a nmo:Call ;" \
+ "nmo:isSent true ;" \
+ "nmo:to ?c ." \
+ "}UNION {" \
+ "?c a nco:Contact ;" \
+ "nco:hasPhoneNumber ?h ." \
+ "?call a nmo:Call ;" \
+ "nmo:from ?c ." \
+ "}" \
+ "}"
+
typedef void (*reply_list_foreach_t) (char **reply, int num_fields,
void *user_data);
@@ -633,6 +687,22 @@ static const char *name2query(const char *name)
return NULL;
}
+static const char *name2count_query(const char *name)
+{
+ if (g_str_equal(name, "telecom/pb.vcf"))
+ return CONTACTS_COUNT_QUERY;
+ else if (g_str_equal(name, "telecom/ich.vcf"))
+ return INCOMING_CALLS_COUNT_QUERY;
+ else if (g_str_equal(name, "telecom/och.vcf"))
+ return OUTGOING_CALLS_COUNT_QUERY;
+ else if (g_str_equal(name, "telecom/mch.vcf"))
+ return MISSED_CALLS_COUNT_QUERY;
+ else if (g_str_equal(name, "telecom/cch.vcf"))
+ return COMBINED_CALLS_COUNT_QUERY;
+
+ return NULL;
+}
+
static gboolean folder_is_valid(const char *folder)
{
if (folder == NULL)
@@ -1022,6 +1092,26 @@ static GString *gen_vcards(GSList *contacts,
return vcards;
}
+static void pull_contacts_size (char **reply, int num_fields, void *user_data)
+{
+ struct phonebook_data *data = user_data;
+
+ if (num_fields < 0) {
+ data->cb(NULL, 0, num_fields, 0, data->user_data);
+ goto fail;
+ }
+
+ if (reply != NULL) {
+ data->index = atoi(reply[0]);
+ return;
+ }
+
+ data->cb(NULL, 0, data->index, 0, data->user_data);
+
+fail:
+ g_free(data);
+}
+
static void pull_contacts(char **reply, int num_fields, void *user_data)
{
struct phonebook_data *data = user_data;
@@ -1284,10 +1374,21 @@ int phonebook_pull(const char *name, const struct apparam_field *params,
{
struct phonebook_data *data;
const char *query;
+ reply_list_foreach_t pull_cb;
+ int col_amount;
DBG("name %s", name);
- query = name2query(name);
+ if (params->maxlistcount == 0) {
+ query = name2count_query(name);
+ col_amount = COUNT_QUERY_COL_AMOUNT;
+ pull_cb = pull_contacts_size;
+ } else {
+ query = name2query(name);
+ col_amount = PULL_QUERY_COL_AMOUNT;
+ pull_cb = pull_contacts;
+ }
+
if (query == NULL)
return -ENOENT;
@@ -1296,7 +1397,7 @@ int phonebook_pull(const char *name, const struct apparam_field *params,
data->user_data = user_data;
data->cb = cb;
- return query_tracker(query, PULL_QUERY_COL_AMOUNT, pull_contacts, data);
+ return query_tracker(query, col_amount, pull_cb, data);
}
int phonebook_get_entry(const char *folder, const char *id,
--
1.7.0.4
^ permalink raw reply related
* [PATCH 1/2] Remove unnecessary empty line in phonebook-tracker
From: Radoslaw Jablonski @ 2010-10-26 8:29 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Radoslaw Jablonski
---
plugins/phonebook-tracker.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
index a6e21f8..277290c 100644
--- a/plugins/phonebook-tracker.c
+++ b/plugins/phonebook-tracker.c
@@ -531,7 +531,6 @@
"}" \
"} GROUP BY ?call ORDER BY DESC(nmo:receivedDate(?call))"
-
#define CONTACTS_QUERY_FROM_URI \
"SELECT ?v nco:fullname(<%s>) " \
"nco:nameFamily(<%s>) nco:nameGiven(<%s>) " \
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH 2/6] Bluetooth: Add LE connect support
From: Ville Tervo @ 2010-10-26 7:55 UTC (permalink / raw)
To: ext Anderson Lizardo; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <AANLkTi=3VB-q-H6vcTAMAC1hYdYVJiF9wVOBKchYEktt@mail.gmail.com>
Hi,
On Mon, Oct 25, 2010 at 10:33:05PM +0200, ext Anderson Lizardo wrote:
> Hi,
>
> On Mon, Oct 25, 2010 at 8:21 AM, Ville Tervo <ville.tervo@nokia.com> wrote:
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index 0b1e460..0944c0c 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -45,6 +45,32 @@
> > #include <net/bluetooth/bluetooth.h>
> > #include <net/bluetooth/hci_core.h>
> >
> > +void hci_le_connect(struct hci_conn *conn)
>
> This function could be made static, right? (just noticed this now)
Yes. Thanks.
>
> > +{
> > + struct hci_dev *hdev = conn->hdev;
> > + struct hci_cp_le_create_conn cp;
> > +
> > + conn->state = BT_CONNECT;
> > + conn->out = 1;
> > +
> > + memset(&cp, 0, sizeof(cp));
> > + cp.scan_interval = cpu_to_le16(0x0004);
> > + cp.scan_window = cpu_to_le16(0x0004);
> > + bacpy(&cp.peer_addr, &conn->dst);
> > + cp.conn_interval_min = cpu_to_le16(0x0008);
> > + cp.conn_interval_max = cpu_to_le16(0x0100);
> > + cp.supervision_timeout = cpu_to_le16(0x0064);
> > + cp.min_ce_len = cpu_to_le16(0x0001);
> > + cp.max_ce_len = cpu_to_le16(0x0001);
> > +
> > + hci_send_cmd(hdev, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
> > +}
--
Ville
^ permalink raw reply
* [PATCH] Add separate queries for listing size in PBAP
From: Radoslaw Jablonski @ 2010-10-26 7:42 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Radoslaw Jablonski
Previosly to get phonebook-size or call history size, full phonebook
query was used and a lot of unnecessary operations need to be done on
that returned data. Now separate query is used to get size of each
listing so performance for "getPhonebookSize" operations improved a lot.
---
plugins/phonebook-tracker.c | 107 ++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 105 insertions(+), 2 deletions(-)
diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
index a6e21f8..1919698 100644
--- a/plugins/phonebook-tracker.c
+++ b/plugins/phonebook-tracker.c
@@ -45,6 +45,7 @@
#define TRACKER_DEFAULT_CONTACT_ME "http://www.semanticdesktop.org/ontologies/2007/03/22/nco#default-contact-me"
#define CONTACTS_ID_COL 38
#define PULL_QUERY_COL_AMOUNT 39
+#define COUNT_QUERY_COL_AMOUNT 1
#define COL_HOME_NUMBER 0
#define COL_HOME_EMAIL 7
#define COL_WORK_NUMBER 8
@@ -580,6 +581,61 @@
"<%s> nco:hasPhoneNumber ?t . " \
"} "
+#define CONTACTS_COUNT_QUERY \
+ "SELECT COUNT(?c) " \
+ "WHERE {" \
+ "?c a nco:PersonContact ." \
+ "FILTER (regex(str(?c), \"contact:\") || " \
+ "regex(str(?c), \"nco#default-contact-me\"))" \
+ "}"
+
+#define MISSED_CALLS_COUNT_QUERY \
+ "SELECT COUNT(?call) WHERE {" \
+ "?c a nco:Contact ;" \
+ "nco:hasPhoneNumber ?h ." \
+ "?call a nmo:Call ;" \
+ "nmo:isSent false ;" \
+ "nmo:from ?c ;" \
+ "nmo:isAnswered false ." \
+ "}"
+
+
+#define INCOMING_CALLS_COUNT_QUERY \
+ "SELECT COUNT(?call) WHERE {" \
+ "?c a nco:Contact ;" \
+ "nco:hasPhoneNumber ?h ." \
+ "?call a nmo:Call ;" \
+ "nmo:isSent false ;" \
+ "nmo:from ?c ;" \
+ "nmo:isAnswered true ." \
+ "}"
+
+#define OUTGOING_CALLS_COUNT_QUERY \
+ "SELECT COUNT(?call) WHERE {" \
+ "?c a nco:Contact ;" \
+ "nco:hasPhoneNumber ?h ." \
+ "?call a nmo:Call ;" \
+ "nmo:isSent true ;" \
+ "nmo:to ?c ." \
+ "}"
+
+
+#define COMBINED_CALLS_COUNT_QUERY \
+ "SELECT COUNT(?call) WHERE {" \
+ "{" \
+ "?c a nco:Contact ;" \
+ "nco:hasPhoneNumber ?h ." \
+ "?call a nmo:Call ;" \
+ "nmo:isSent true ;" \
+ "nmo:to ?c ." \
+ "}UNION {" \
+ "?c a nco:Contact ;" \
+ "nco:hasPhoneNumber ?h ." \
+ "?call a nmo:Call ;" \
+ "nmo:from ?c ." \
+ "}" \
+ "}"
+
typedef void (*reply_list_foreach_t) (char **reply, int num_fields,
void *user_data);
@@ -634,6 +690,22 @@ static const char *name2query(const char *name)
return NULL;
}
+static const char *name2count_query(const char *name)
+{
+ if (g_str_equal(name, "telecom/pb.vcf"))
+ return CONTACTS_COUNT_QUERY;
+ else if (g_str_equal(name, "telecom/ich.vcf"))
+ return INCOMING_CALLS_COUNT_QUERY;
+ else if (g_str_equal(name, "telecom/och.vcf"))
+ return OUTGOING_CALLS_COUNT_QUERY;
+ else if (g_str_equal(name, "telecom/mch.vcf"))
+ return MISSED_CALLS_COUNT_QUERY;
+ else if (g_str_equal(name, "telecom/cch.vcf"))
+ return COMBINED_CALLS_COUNT_QUERY;
+
+ return NULL;
+}
+
static gboolean folder_is_valid(const char *folder)
{
if (folder == NULL)
@@ -1023,6 +1095,26 @@ static GString *gen_vcards(GSList *contacts,
return vcards;
}
+static void pull_contacts_size (char **reply, int num_fields, void *user_data)
+{
+ struct phonebook_data *data = user_data;
+
+ if (num_fields < 0) {
+ data->cb(NULL, 0, num_fields, 0, data->user_data);
+ goto fail;
+ }
+
+ if (reply != NULL) {
+ data->index = atoi(reply[0]);
+ return;
+ }
+
+ data->cb(NULL, 0, data->index, 0, data->user_data);
+
+fail:
+ g_free(data);
+}
+
static void pull_contacts(char **reply, int num_fields, void *user_data)
{
struct phonebook_data *data = user_data;
@@ -1285,10 +1377,21 @@ int phonebook_pull(const char *name, const struct apparam_field *params,
{
struct phonebook_data *data;
const char *query;
+ reply_list_foreach_t pull_cb;
+ int col_amount;
DBG("name %s", name);
- query = name2query(name);
+ if (params->maxlistcount == 0) {
+ query = name2count_query(name);
+ col_amount = COUNT_QUERY_COL_AMOUNT;
+ pull_cb = pull_contacts_size;
+ } else {
+ query = name2query(name);
+ col_amount = PULL_QUERY_COL_AMOUNT;
+ pull_cb = pull_contacts;
+ }
+
if (query == NULL)
return -ENOENT;
@@ -1297,7 +1400,7 @@ int phonebook_pull(const char *name, const struct apparam_field *params,
data->user_data = user_data;
data->cb = cb;
- return query_tracker(query, PULL_QUERY_COL_AMOUNT, pull_contacts, data);
+ return query_tracker(query, col_amount, pull_cb, data);
}
int phonebook_get_entry(const char *folder, const char *id,
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH 1/2 v2] Bluetooth: Fix system crash caused by del_timer()
From: haijun liu @ 2010-10-26 7:35 UTC (permalink / raw)
To: shy; +Cc: Gustavo F. Padovan, Haijun Liu, linux-bluetooth
In-Reply-To: <AANLkTin+dNkjySQBvCSLK9f5aRF9445UqjhXaNvKWSz_@mail.gmail.com>
Hi Shy,
> You can check the sk in list l2cap_sk_list.
If we use global lock, in case of mp env., we have to keep it locked
before exiting l2cap_monitor_timeout().
I don't know whether it can be accepted.
--
Haijun Liu
^ permalink raw reply
* Re: [PATCH 1/9] mfd: Add support for the ST-Ericsson CG2900.
From: Par-Gunnar Hjalmdahl @ 2010-10-26 6:54 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linus.walleij, linux-bluetooth, linux-kernel
In-Reply-To: <201010221736.57671.arnd@arndb.de>
Hi Arnd,
Thanks for your comments. See below for answers.
/P-G
2010/10/22 Arnd Bergmann <arnd@arndb.de>:
> On Friday 22 October 2010 12:35:16 Par-Gunnar Hjalmdahl wrote:
>> This patch adds support for the ST-Ericsson CG2900 Connectivity
>> Combo controller.
>> This patch adds the central framework to be able to register CG2900 users,
>> transports, and chip handlers.
>>
>> Signed-off-by: Par-Gunnar Hjalmdahl <par-gunnar.p.hjalmdahl@stericsson.com>
>
> Looks ok from a coding style perspective, but some important information is
> missing from the description:
>
> * What is a CG2900?
> * Why is it a MFD device rather than e.g. a bus or a subsystem?
>
I will add some more info:
- CG2900 is a GPS, Bluetooth, FM controller
- I do not really know what makes a device qualify as a certain type,
but at least for us this is a multifunction device. But I can't really
say that it isn't really a bus (could be stated as multifunction HCI
bus). I guess it could be qualified as a subsystem as well, but I
cannot give you a reason to have it as either.
>> +config MFD_CG2900
>> + tristate "Support ST-Ericsson CG2900 main structure"
>> + depends on NET
>> + help
>> + Support for ST-Ericsson CG2900 Connectivity Combo controller main structure.
>> + Supports multiple functionalities muxed over a Bluetooth HCI H:4 interface.
>> + CG2900 support Bluetooth, FM radio, and GPS.
>> +
>
> Can you explain what it means to mux over a H:4 interface? Does this mean
> you use bluetooth infrastructure that is designed for wireless communication
> in order to connect on-board or on-chip devices?
>
The H:4 protocol is defined in the Bluetooth Core specification. It
uses a single byte first in the data packet to determine an HCI
channel. In the Bluetooth Core specification there are 4 channels
specified, 0x01-0x04 (0x01 is BT command, etc). The rest of the
channels are reserved, but these have been used on a proprietary basis
to transport data to different IPs within the controller. In our API
we have chosen to have a channel separation on an API basis and the
H:4 byte is then added within the driver. So we have multiple channels
coming in from the users that we mux onto a single data transport.
That's what I mean in the text. I guess I could rewrite it a bit to
make it clearer.
>> @@ -0,0 +1,2401 @@
>> +/*
>> + * drivers/mfd/cg2900/cg2900_core.c
>> + *
>> + * Copyright (C) ST-Ericsson SA 2010
>> + * Authors:
>> + * Par-Gunnar Hjalmdahl (par-gunnar.p.hjalmdahl@stericsson.com) for
>> ST-Ericsson.
>
> Your email client rewraps lines. You need to fix so that other people
> can apply your patches.
>
I will fix this.
>> +/*
>> + * chip_handlers - List of the register handlers for different chips.
>> + */
>> +LIST_HEAD(chip_handlers);
>
> Should this be static? Don't you need a lock to access the list?
>
You're right. I need to have mutex for this and it can just as well be
in the allocated cg2900_info structure.
>> +/**
>> + * find_h4_user() - Get H4 user based on supplied H4 channel.
>> + * @h4_channel: H4 channel.
>> + * @dev: Stored CG2900 device.
>> + * @skb: (optional) skb with received packet. Set to NULL if NA.
>> + *
>> + * Returns:
>> + * 0 if there is no error.
>> + * -EINVAL if bad channel is supplied or no user was found.
>> + * -ENXIO if channel is audio channel but not registered with CG2900.
>> + */
>> +static int find_h4_user(int h4_channel, struct cg2900_device **dev,
>> + const struct sk_buff * const skb)
>> +{
>> + int err = 0;
>> + struct cg2900_users *users = &(core_info->users);
>> + struct cg2900_h4_channels *chan = &(core_info->h4_channels);
>> +
>> + if (h4_channel == chan->bt_cmd_channel) {
>> + *dev = users->bt_cmd;
>> + } else if (h4_channel == chan->bt_acl_channel) {
>> + *dev = users->bt_acl;
>> + } else if (h4_channel == chan->bt_evt_channel) {
>> + *dev = users->bt_evt;
>> + /* Check if it's event generated by previously sent audio user
>> + * command. If so then that event should be dispatched to audio
>> + * user*/
>> + err = find_bt_audio_user(h4_channel, dev, skb);
>> + } else if (h4_channel == chan->gnss_channel) {
>> + *dev = users->gnss;
>> + } else if (h4_channel == chan->fm_radio_channel) {
>> + *dev = users->fm_radio;
>> + /* Check if it's an event generated by previously sent audio
>> + * user command. If so then that event should be dispatched to
>> + * audio user */
>> + err = find_fm_audio_user(h4_channel, dev, skb);
>> + } else if (h4_channel == chan->debug_channel) {
>> + *dev = users->debug;
>> + } else if (h4_channel == chan->ste_tools_channel) {
>> + *dev = users->ste_tools;
>> + } else if (h4_channel == chan->hci_logger_channel) {
>> + *dev = users->hci_logger;
>> + } else if (h4_channel == chan->us_ctrl_channel) {
>> + *dev = users->us_ctrl;
>> + } else if (h4_channel == chan->core_channel) {
>> + *dev = users->core;
>> + } else {
>> + *dev = NULL;
>> + CG2900_ERR("Bad H:4 channel supplied: 0x%X", h4_channel);
>> + return -EINVAL;
>> + }
>> +
>> + return err;
>> +}
>
> You seem to have a number of functions that need to go through each
> possible user/channel/submodule. This looks like somethin is not
> abstracted well enough.
>
This is part of what I stated in the cover letter "Future
development". We will move this functionality out to each chip handler
file. I agree that current code is not the perfect abstraction
level... ;-)
>> +
>> +/**
>> + * add_h4_user() - Add H4 user to user storage based on supplied H4 channel.
>> + * @dev: Stored CG2900 device.
>> + * @name: Device name to identify different devices that are using
>> + * the same H4 channel.
>> + *
>> + * Returns:
>> + * 0 if there is no error.
>> + * -EINVAL if NULL pointer or bad channel is supplied.
>> + * -EBUSY if there already is a user for supplied channel.
>> + */
>> +static int add_h4_user(struct cg2900_device *dev, const char * const name)
>> +{
>
> Where does that name come from? Why not just use an enum if the name is
> only use for strncmp?
>
At a point in time we used to have an enum, but from what I can see it
is easier to keep an API stable if you use name lookup instead. If we
want to have minimal changes to the API in the future this is quite a
flexible solution, but this code should be moved to each respective
chip handler.
>> +static ssize_t test_char_dev_read(struct file *filp, char __user *buf,
>> + size_t count, loff_t *f_pos)
>> +{
>> + struct sk_buff *skb;
>> + int bytes_to_copy;
>> + int err;
>> + struct sk_buff_head *rx_queue = &core_info->test_char_dev->rx_queue;
>
> What is this read/write interface used for?
>
> The name suggests that this is only for testing and not an essential
> part of your driver. Could this be made a separate driver?
>
> It also looks like you do socket communication here, can't you use
> a proper AF_BLUETOOTH socket to do the same?
>
This interface can be used for module testing where you can have a
test engine in user space that acts as a chip. Yes, it could be made
into a separate driver but it would be a very small driver. Don't know
if it will be worth having it in a separate file...
We use the sk_buffer, which I guess is the reason that you mention
sockets. We could of course use a socket instead of a char dev, both
here and in the cg2900_char_dev file (which of course then would be
renamed). It was just a choice we made during development. We think
that the transport as such is more like a streaming interface, but I
see no real problem to have sockets. We already have several users
using char dev though so I would prefer to keep char dev rather than
switching to sockets unless you have a strong reason for this.
I see no reason to use AF_BLUETOOTH though, the transport is not only
for Bluetooth.
>> + CG2900_INFO("test_char_dev_read");
>> +
>> + if (skb_queue_empty(rx_queue))
>> + wait_event_interruptible(char_wait_queue,
>> + !(skb_queue_empty(rx_queue)));
>> +
>> + skb = skb_dequeue(rx_queue);
>> + if (!skb) {
>> + CG2900_INFO("skb queue is empty - return with zero bytes");
>> + bytes_to_copy = 0;
>> + goto finished;
>> + }
>> +
>> + bytes_to_copy = min(count, skb->len);
>> + err = copy_to_user(buf, skb->data, bytes_to_copy);
>> + if (err) {
>> + skb_queue_head(rx_queue, skb);
>> + return -EFAULT;
>> + }
>> +
>> + skb_pull(skb, bytes_to_copy);
>> +
>> + if (skb->len > 0)
>> + skb_queue_head(rx_queue, skb);
>> + else
>> + kfree_skb(skb);
>> +
>> +finished:
>> + return bytes_to_copy;
>> +}
>
> This does not handle short/continued reads.
>
As I mentioned above this interface is used for testing and we
therefore have some restriction of usage. I don't think we need to
impose all things necessary for a full interface upon it.
>> +EXPORT_SYMBOL(cg2900_reset);
>
> How about making your symbols EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL?
>
I have no problems with this. I will check with our company policy
first though before I give a final answer.
But as far as I can see we can use EXPORT_SYMBOL_GPL.
>> +module_param(cg2900_debug_level, int, S_IRUGO | S_IWUSR | S_IWGRP);
>> +MODULE_PARM_DESC(cg2900_debug_level,
>> + "Debug level. Default 1. Possible values:\n"
>> + "\t0 = No debug\n"
>> + "\t1 = Error prints\n"
>> + "\t10 = General info, e.g. function entries\n"
>> + "\t20 = Debug info, e.g. steps in a functionality\n"
>> + "\t25 = Data info, i.e. prints when data is transferred\n"
>> + "\t30 = Data content, i.e. contents of the transferred data");
>
> Please don't introduce new debugging frameworks, we have enough of them
> already. Syslog handles different levels of output just fine, so just
> remove all your debugging stuff and use dev_dbg, dev_info, etc to print
> messages about the device if you must.
>
> Many messages can probably be removed entirely.
>
I have already received comments for this so this will be remade, or
rather removed...
>> +#define CG2900_DEFAULT_DEBUG_LEVEL 1
>> +
>> +/* module_param declared in cg2900_core.c */
>> +extern int cg2900_debug_level;
>> +
>> +#if defined(NDEBUG) || CG2900_DEFAULT_DEBUG_LEVEL == 0
>> + #define CG2900_DBG_DATA_CONTENT(__prefix, __buf, __len)
>> + #define CG2900_DBG_DATA(fmt, arg...)
>> + #define CG2900_DBG(fmt, arg...)
>> + #define CG2900_INFO(fmt, arg...)
>> + #define CG2900_ERR(fmt, arg...)
>> +#else
>> +
>> + #define CG2900_DBG_DATA_CONTENT(__prefix, __buf, __len) \
>> + do { \
>> + if (cg2900_debug_level >= 30) \
>> + print_hex_dump_bytes("CG2900 " __prefix ": " , \
>> + DUMP_PREFIX_NONE, __buf, __len); \
>> + } while (0)
>> +
>> + #define CG2900_DBG_DATA(fmt, arg...) \
>> + do { \
>> + if (cg2900_debug_level >= 25) \
>> + pr_debug("CG2900 %s: " fmt "\n" , __func__ , ## arg); \
>> + } while (0)
>> +
>> + #define CG2900_DBG(fmt, arg...) \
>> + do { \
>> + if (cg2900_debug_level >= 20) \
>> + pr_debug("CG2900 %s: " fmt "\n" , __func__ , ## arg); \
>> + } while (0)
>> +
>> + #define CG2900_INFO(fmt, arg...) \
>> + do { \
>> + if (cg2900_debug_level >= 10) \
>> + pr_info("CG2900: " fmt "\n" , ## arg); \
>> + } while (0)
>> +
>> + #define CG2900_ERR(fmt, arg...) \
>> + do { \
>> + if (cg2900_debug_level >= 1) \
>> + pr_err("CG2900 %s: " fmt "\n" , __func__ , ## arg); \
>> + } while (0)
>> +
>> +#endif /* NDEBUG */
>
> You'll feel relieved when all this is gone...
>
The only thing is it's been pretty nice to have it, but I will remove it.
Is it OK to keep defines so we can have "CG2900" in front and "\n"
after the text?
>> +
>> +#ifndef _BLUETOOTH_DEFINES_H_
>> +#define _BLUETOOTH_DEFINES_H_
>> +
>> +#include <linux/types.h>
>> +
>> +/* H:4 offset in an HCI packet */
>> +#define HCI_H4_POS 0
>> +#define HCI_H4_SIZE 1
>> +
>> +/* Standardized Bluetooth H:4 channels */
>> +#define HCI_BT_CMD_H4_CHANNEL 0x01
>> +#define HCI_BT_ACL_H4_CHANNEL 0x02
>> +#define HCI_BT_SCO_H4_CHANNEL 0x03
>> +#define HCI_BT_EVT_H4_CHANNEL 0x04
>> +
>> +/* Bluetooth Opcode Group Field (OGF) */
>> +#define HCI_BT_OGF_LINK_CTRL 0x01
>> +#define HCI_BT_OGF_LINK_POLICY 0x02
>> +#define HCI_BT_OGF_CTRL_BB 0x03
>> +#define HCI_BT_OGF_LINK_INFO 0x04
>> +#define HCI_BT_OGF_LINK_STATUS 0x05
>> +#define HCI_BT_OGF_LINK_TESTING 0x06
>> +#define HCI_BT_OGF_VS 0x3F
>
> These all look like generic bluetooth definitions, not cg2900
> specific. Should they be part of global headers? Are they perhaps
> already?
>
> Arnd
>
The H4 defines are standardized values and can be in a common h-file.
I don't know if they should be in e.g. include/net/bluetooth/hci.h,
but there are no other H4 defines there and I'm not sure if they would
fit there. Regarding the last defines (the OGFs) they should fit in
hci.h, but I don't know if anyone else would use them. I'm not
actually sure that we use them ourselves anymore so we might be able
to just remove them.
In short, I will check this up and change as needed.
^ permalink raw reply
* Re: [RFC] LE connections and advertising management
From: Claudio Takahasi @ 2010-10-26 2:51 UTC (permalink / raw)
To: Brian Redding; +Cc: BlueZ development
In-Reply-To: <000001cb747a$9fd61270$df823750$@org>
Hi Brian,
On Mon, Oct 25, 2010 at 4:27 PM, Brian Redding <bredding@codeaurora.org> wrote:
> Hi Claudio,
>
> Are there still interfaces that need to be added to attribute-api.txt to handle client and server characteristic configuration as well as presentation and aggregate formats? I see those as TODO items but wondered if the APIs for them have been defined yet.
>
> One thing to note on the server API is that a GATT-based profile could specify behavior on a characteristic value that when the characteristic value is read to perform some action in a similar way as a hardware register. It appears that the notes I'm reading in the code thus far only consider changes (or writes) to characteristic values for the watch code.
>
> Also does the current API handle included services?
>
> The Bluetooth SIG is beginning to look at 3rd party application developer API's for both client and servers for various platforms so understanding what is currently defined in BlueZ and what still needs to be added would be useful.
>
> Thanks,
> Brian
> ---
> Brian A. Redding
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
The API to address characteristic descriptors is still being defined.
We are focusing in the advertising and connection management at the
moment. If you have suggestion of how to represent the descriptors in
the attribute API, suggestions are welcome!
There isn't a server API at the moment, we implemented a server for
testing purpose only. But we will need to define it soon.
Which pages/section of the spec describes this read characteristic behavior?
Included services are not supported by our client. How important is
it? It is mandatory for qualification?
Regards,
Claudio.
^ permalink raw reply
* Re: [PATCH 1/2 v2] Bluetooth: Fix system crash caused by del_timer()
From: haijun liu @ 2010-10-26 1:32 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: Haijun Liu, linux-bluetooth
In-Reply-To: <20101025110131.GA7721@vigoh>
Hi Gustavo,
>> >> During test session with another vendor's bt stack, found that in
>> >> l2cap_chan_del() using del_timer() caused l2cap_monitor_timeout()
>> >> be called after the sock was freed, so it raised a system crash.
>> >> So I just replaced del_timer() with del_timer_sync() to solve it.
>> >
>> > NAK on this. If you read the del_timer_sync() documentation you can
>> > see that you can't call del_timer_sync() on interrupt context. The
>> > possible solution here is to check in the beginning of
>> > l2cap_monitor_timeout() if your sock is still valid.
>> >
>>
>> You are right, I only considered close() interface, so missed the interrupt
>> context.
>>
>> It's very difficult to check sock valid or not in timeout procedure, since it's
>> an interrupt context, and only can get context from parameter pre-stored,
>> except global variables.
>
> I think you can check for sk == null there.
>
It's a pre-stored parameter, it will not change by itself.
--
Haijun Liu
^ permalink raw reply
* Re: 4.76 possible regression: bluetoothd segfaults when launching bluetooth programs
From: Johan Hedberg @ 2010-10-25 20:40 UTC (permalink / raw)
To: Ilya Basin; +Cc: linux-bluetooth
In-Reply-To: <453819375.20101024163802@gmail.com>
Hi Ilya,
On Sun, Oct 24, 2010, Ilya Basin wrote:
> Program received signal SIGSEGV, Segmentation fault.
> 0xb7d3e653 in strlen () from /lib/libc.so.6
> (gdb) bt
> #0 0xb7d3e653 in strlen () from /lib/libc.so.6
> #1 0xb7e5eb10 in ?? () from /usr/lib/libdbus-1.so.3
> #2 0xb7e4a34b in ?? () from /usr/lib/libdbus-1.so.3
> #3 0xb7e4e7a9 in dbus_message_iter_append_basic () from /usr/lib/libdbus-1.so.3
> #4 0xb7fef03d in append_array_variant ()
> #5 0xb7fef799 in emit_array_property_changed ()
> #6 0xb7fe4de4 in adapter_service_ins_rem ()
> #7 0xb7fd7fb1 in sdp_record_add ()
> #8 0xb7fd79de in service_register_req ()
> #9 0xb7fd5dfc in handle_request ()
> #10 0xb7fd496e in io_session_event ()
> #11 0xb7ef7a2b in ?? () from /usr/lib/libglib-2.0.so.0
> #12 0xb7eb0b72 in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
> #13 0xb7eb1350 in ?? () from /usr/lib/libglib-2.0.so.0
> #14 0xb7eb1a1b in g_main_loop_run () from /usr/lib/libglib-2.0.so.0
> #15 0xb7fd1bbd in main ()
> (gdb)
Unfortunately this doesn't give too much info since you don't seem to
have all debug symbols enabled. Could you try to reproduce this with
latest bluez git. You don't need to install anything but just compile
(./boostrap-configure && make) and run (src/bluetoothd -nd) from the
source tree directly. Then, it'd also be nice if you could use git
bisect to determine the exact commit between 4.69 and 4.76 that
introduced this regression.
Thanks.
Johan
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox