2012-05-07 23:20:22

by Andrzej Kaczmarek

[permalink] [raw]
Subject: [PATCH] RFC: LE Connection Complete has only status parameter set

Hi,

I noticed that my BLE dongle does not return peer address in LE Connection
Complete event after connection attempt was cancelled with LE Create Connection
Cancel command, as follows:

2012-05-07 11:21:39.133378 < HCI Command: LE Create Connection (0x08|0x000d) plen 25
bdaddr 00:22:D0:10:13:EE type 1
2012-05-07 11:21:39.138774 > HCI Event: Command Status (0x0f) plen 4
LE Create Connection (0x08|0x000d) status 0x00 ncmd 1
2012-05-07 11:21:44.752854 < HCI Command: LE Create Connection Cancel (0x08|0x000e) plen 0
2012-05-07 11:21:44.759475 > HCI Event: Command Complete (0x0e) plen 4
LE Create Connection Cancel (0x08|0x000e) ncmd 1
2012-05-07 11:21:44.764479 > HCI Event: LE Meta Event (0x3e) plen 19
LE Connection Complete
status 0x02 handle 0, role master
bdaddr 00:00:00:00:00:00 (Public)

This causes problems in kernel since hci_conn is not properly removed and
subsequent connections to this peer are not possible - since connection with
peer 00:00:00:00:00:00 does not exist, it will be created and immediately
removed (due to non-zero status code) leaving original hci_conn intact:

[14898.739425] [6603] hci_connect: hci0 dst 00:22:D0:10:13:EE
[14898.739429] [6603] hci_conn_add: hci0 dst 00:22:D0:10:13:EE
[14898.739434] [6603] hci_conn_init_sysfs: conn ffff880079f03000
[14898.739440] [6603] hci_send_cmd: hci0 opcode 0x200d plen 25
[14898.739443] [6603] hci_send_cmd: skb len 28
[14898.739487] [6603] hci_chan_create: hci0 conn ffff880079f03000
...
[14938.860231] [55] hci_send_cmd: hci0 opcode 0x200e plen 0
...
[14938.876427] [55] hci_le_conn_complete_evt: hci0 status 2
[14938.876433] [55] hci_conn_add: hci0 dst 00:00:00:00:00:00
[14938.876439] [55] hci_conn_init_sysfs: conn ffff88007aeff800
[14938.876454] [55] hci_send_to_control: len 14
[14938.876470] [55] l2cap_connect_cfm: hcon ffff88007aeff800 bdaddr 00:00:00:00:00:00 status 2
[14938.876474] [55] hci_conn_del: hci0 conn ffff88007aeff800 handle 0


I'm not sure if behaviour of BLE dongle is correct but I'd say yes since
Bluetooth spec states in part E section 7.8.13 as follows:
The LE Connection Complete event with the error code Unknown Connection
Identifier (0x02) shall be sent after the Command Complete event for the
LE_Create_Connection_Cancel command if the cancellation was successful.

This is what is returned. And since we can have only one LE connection in
BT_CONNECT state we can safely handle failed LE Connection Complete event
based on what is stored in existing in hci_conn rather than in event. This
is what my following patch does.

BLE dongle used:
P: Vendor=0a5c ProdID=21e8 Rev=01.12
S: Manufacturer=Broadcom Corp
S: Product=BCM20702A0
S: SerialNumber=000272D66A3F


Andrzej Kaczmarek (1):
Bluetooth: Use hci_conn data to handle failed LE Connection Complete

net/bluetooth/hci_event.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)

--
1.7.9.5



2012-05-16 19:59:03

by Gustavo Padovan

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: Use hci_conn data to handle failed LE Connection Complete

* Andrzej Kaczmarek <[email protected]> [2012-05-08 01:20:23 +0200]:

> This patch changes way LE Connection Complete event with error status are
> handled by reusing peer information stored in hci_conn instead of these
> returned in event packet which may not be always valid.
>
> LE Connection Complete event for failed (cancelled) connection does not
> include peer address on some Bluetooth chips and only status parameter is
> filled. In such case appriopriate hci_conn is not removed and subsequent
> connections to given peer won't be possible.
>
> 2012-05-07 11:21:39.133378 < HCI Command: LE Create Connection (0x08|0x000d) plen 25
> bdaddr 00:22:D0:10:13:EE type 1
> 2012-05-07 11:21:39.138774 > HCI Event: Command Status (0x0f) plen 4
> LE Create Connection (0x08|0x000d) status 0x00 ncmd 1
> 2012-05-07 11:21:44.752854 < HCI Command: LE Create Connection Cancel (0x08|0x000e) plen 0
> 2012-05-07 11:21:44.759475 > HCI Event: Command Complete (0x0e) plen 4
> LE Create Connection Cancel (0x08|0x000e) ncmd 1
> 2012-05-07 11:21:44.764479 > HCI Event: LE Meta Event (0x3e) plen 19
> LE Connection Complete
> status 0x02 handle 0, role master
> bdaddr 00:00:00:00:00:00 (Public)
>
> [14898.739425] [6603] hci_connect: hci0 dst 00:22:D0:10:13:EE
> [14898.739429] [6603] hci_conn_add: hci0 dst 00:22:D0:10:13:EE
> [14898.739434] [6603] hci_conn_init_sysfs: conn ffff880079f03000
> [14898.739440] [6603] hci_send_cmd: hci0 opcode 0x200d plen 25
> [14898.739443] [6603] hci_send_cmd: skb len 28
> [14898.739487] [6603] hci_chan_create: hci0 conn ffff880079f03000
> ...
> [14938.860231] [55] hci_send_cmd: hci0 opcode 0x200e plen 0
> ...
> [14938.876427] [55] hci_le_conn_complete_evt: hci0 status 2
> [14938.876433] [55] hci_conn_add: hci0 dst 00:00:00:00:00:00
> [14938.876439] [55] hci_conn_init_sysfs: conn ffff88007aeff800
> [14938.876454] [55] hci_send_to_control: len 14
> [14938.876470] [55] l2cap_connect_cfm: hcon ffff88007aeff800 bdaddr 00:00:00:00:00:00 status 2
> [14938.876474] [55] hci_conn_del: hci0 conn ffff88007aeff800 handle 0
>
> Signed-off-by: Andrzej Kaczmarek <[email protected]>
> ---
> net/bluetooth/hci_event.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index b739baf..6ddb79e 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3300,6 +3300,19 @@ static inline void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff
>
> hci_dev_lock(hdev);
>
> + if (ev->status) {
> + conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> + if (!conn)
> + goto unlock;
> +
> + mgmt_connect_failed(hdev, &conn->dst, conn->type,
> + conn->dst_type, ev->status);

Actually I unpulled it from the tree, the coding style is wrong. When you
break lines in functions call/definition or ifs you should align the following
line with the opening parenthesis.

Gustavo

2012-05-16 19:25:35

by Gustavo Padovan

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: Use hci_conn data to handle failed LE Connection Complete

Hi Andrzej,

* Andrzej Kaczmarek <[email protected]> [2012-05-08 01:20:23 +0200]:

> This patch changes way LE Connection Complete event with error status are
> handled by reusing peer information stored in hci_conn instead of these
> returned in event packet which may not be always valid.
>
> LE Connection Complete event for failed (cancelled) connection does not
> include peer address on some Bluetooth chips and only status parameter is
> filled. In such case appriopriate hci_conn is not removed and subsequent
> connections to given peer won't be possible.
>
> 2012-05-07 11:21:39.133378 < HCI Command: LE Create Connection (0x08|0x000d) plen 25
> bdaddr 00:22:D0:10:13:EE type 1
> 2012-05-07 11:21:39.138774 > HCI Event: Command Status (0x0f) plen 4
> LE Create Connection (0x08|0x000d) status 0x00 ncmd 1
> 2012-05-07 11:21:44.752854 < HCI Command: LE Create Connection Cancel (0x08|0x000e) plen 0
> 2012-05-07 11:21:44.759475 > HCI Event: Command Complete (0x0e) plen 4
> LE Create Connection Cancel (0x08|0x000e) ncmd 1
> 2012-05-07 11:21:44.764479 > HCI Event: LE Meta Event (0x3e) plen 19
> LE Connection Complete
> status 0x02 handle 0, role master
> bdaddr 00:00:00:00:00:00 (Public)
>
> [14898.739425] [6603] hci_connect: hci0 dst 00:22:D0:10:13:EE
> [14898.739429] [6603] hci_conn_add: hci0 dst 00:22:D0:10:13:EE
> [14898.739434] [6603] hci_conn_init_sysfs: conn ffff880079f03000
> [14898.739440] [6603] hci_send_cmd: hci0 opcode 0x200d plen 25
> [14898.739443] [6603] hci_send_cmd: skb len 28
> [14898.739487] [6603] hci_chan_create: hci0 conn ffff880079f03000
> ...
> [14938.860231] [55] hci_send_cmd: hci0 opcode 0x200e plen 0
> ...
> [14938.876427] [55] hci_le_conn_complete_evt: hci0 status 2
> [14938.876433] [55] hci_conn_add: hci0 dst 00:00:00:00:00:00
> [14938.876439] [55] hci_conn_init_sysfs: conn ffff88007aeff800
> [14938.876454] [55] hci_send_to_control: len 14
> [14938.876470] [55] l2cap_connect_cfm: hcon ffff88007aeff800 bdaddr 00:00:00:00:00:00 status 2
> [14938.876474] [55] hci_conn_del: hci0 conn ffff88007aeff800 handle 0
>
> Signed-off-by: Andrzej Kaczmarek <[email protected]>
> ---
> net/bluetooth/hci_event.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)

Patch has been applied to bluetooth-next, thanks.

Gustavo

2012-05-16 18:23:15

by Andre Guedes

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: Use hci_conn data to handle failed LE Connection Complete

Hi Andrzej,

On Mon, May 7, 2012 at 8:20 PM, Andrzej Kaczmarek
<[email protected]> wrote:
> This patch changes way LE Connection Complete event with error status are
> handled by reusing peer information stored in hci_conn instead of these
> returned in event packet which may not be always valid.
>
> LE Connection Complete event for failed (cancelled) connection does not
> include peer address on some Bluetooth chips and only status parameter is
> filled. In such case appriopriate hci_conn is not removed and subsequent
> connections to given peer won't be possible.

Some firmwares implementation (I've seen this in some Broadcom and
ST-Ericsson dongles so far) don't include the address in the LE
Connection Complete Event in the case of a cancel. The reason is that
it is also possible to connect using the whitelist (a set of BD
addresses), and in that case, it would not even be possible to include
an address. Thus we should not assume that it contains a meaningful
address.

> 2012-05-07 11:21:39.133378 < HCI Command: LE Create Connection (0x08|0x000d) plen 25
> ? ?bdaddr 00:22:D0:10:13:EE type 1
> 2012-05-07 11:21:39.138774 > HCI Event: Command Status (0x0f) plen 4
> ? ?LE Create Connection (0x08|0x000d) status 0x00 ncmd 1
> 2012-05-07 11:21:44.752854 < HCI Command: LE Create Connection Cancel (0x08|0x000e) plen 0
> 2012-05-07 11:21:44.759475 > HCI Event: Command Complete (0x0e) plen 4
> ? ?LE Create Connection Cancel (0x08|0x000e) ncmd 1
> 2012-05-07 11:21:44.764479 > HCI Event: LE Meta Event (0x3e) plen 19
> ? ?LE Connection Complete
> ? ? ?status 0x02 handle 0, role master
> ? ? ?bdaddr 00:00:00:00:00:00 (Public)
>
> [14898.739425] [6603] hci_connect: hci0 dst 00:22:D0:10:13:EE
> [14898.739429] [6603] hci_conn_add: hci0 dst 00:22:D0:10:13:EE
> [14898.739434] [6603] hci_conn_init_sysfs: conn ffff880079f03000
> [14898.739440] [6603] hci_send_cmd: hci0 opcode 0x200d plen 25
> [14898.739443] [6603] hci_send_cmd: skb len 28
> [14898.739487] [6603] hci_chan_create: hci0 conn ffff880079f03000
> ...
> [14938.860231] [55] hci_send_cmd: hci0 opcode 0x200e plen 0
> ...
> [14938.876427] [55] hci_le_conn_complete_evt: hci0 status 2
> [14938.876433] [55] hci_conn_add: hci0 dst 00:00:00:00:00:00
> [14938.876439] [55] hci_conn_init_sysfs: conn ffff88007aeff800
> [14938.876454] [55] hci_send_to_control: len 14
> [14938.876470] [55] l2cap_connect_cfm: hcon ffff88007aeff800 bdaddr 00:00:00:00:00:00 status 2
> [14938.876474] [55] hci_conn_del: hci0 conn ffff88007aeff800 handle 0
>
> Signed-off-by: Andrzej Kaczmarek <[email protected]>
> ---
> ?net/bluetooth/hci_event.c | ? 22 +++++++++++++---------
> ?1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index b739baf..6ddb79e 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3300,6 +3300,19 @@ static inline void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff
>
> ? ? ? ?hci_dev_lock(hdev);
>
> + ? ? ? if (ev->status) {
> + ? ? ? ? ? ? ? conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> + ? ? ? ? ? ? ? if (!conn)
> + ? ? ? ? ? ? ? ? ? ? ? goto unlock;
> +
> + ? ? ? ? ? ? ? mgmt_connect_failed(hdev, &conn->dst, conn->type,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? conn->dst_type, ev->status);
> + ? ? ? ? ? ? ? hci_proto_connect_cfm(conn, ev->status);
> + ? ? ? ? ? ? ? conn->state = BT_CLOSED;
> + ? ? ? ? ? ? ? hci_conn_del(conn);
> + ? ? ? ? ? ? ? goto unlock;
> + ? ? ? }
> +
> ? ? ? ?conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &ev->bdaddr);
> ? ? ? ?if (!conn) {
> ? ? ? ? ? ? ? ?conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
> @@ -3312,15 +3325,6 @@ static inline void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff
> ? ? ? ? ? ? ? ?conn->dst_type = ev->bdaddr_type;
> ? ? ? ?}
>
> - ? ? ? if (ev->status) {
> - ? ? ? ? ? ? ? mgmt_connect_failed(hdev, &ev->bdaddr, conn->type,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? conn->dst_type, ev->status);
> - ? ? ? ? ? ? ? hci_proto_connect_cfm(conn, ev->status);
> - ? ? ? ? ? ? ? conn->state = BT_CLOSED;
> - ? ? ? ? ? ? ? hci_conn_del(conn);
> - ? ? ? ? ? ? ? goto unlock;
> - ? ? ? }
> -
> ? ? ? ?if (!test_and_set_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags))
> ? ? ? ? ? ? ? ?mgmt_device_connected(hdev, &ev->bdaddr, conn->type,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?conn->dst_type, 0, NULL, 0, NULL);
> --
> 1.7.9.5

Since we can only have one ongoing LE connection initiation at time,
checking hci_conn in state BT_CONNECT is enough.

The patch looks good to me.

Acked-by: Andre Guedes <[email protected]>

BR,

Andre

2012-05-08 22:21:44

by Claudio Takahasi

[permalink] [raw]
Subject: Re: [PATCH] RFC: LE Connection Complete has only status parameter set

Hi Andrzej,

On Mon, May 7, 2012 at 8:20 PM, Andrzej Kaczmarek
<[email protected]> wrote:
> Hi,
>
> I noticed that my BLE dongle does not return peer address in LE Connection
> Complete event after connection attempt was cancelled with LE Create Connection
> Cancel command, as follows:
>
> 2012-05-07 11:21:39.133378 < HCI Command: LE Create Connection (0x08|0x000d) plen 25
>    bdaddr 00:22:D0:10:13:EE type 1
> 2012-05-07 11:21:39.138774 > HCI Event: Command Status (0x0f) plen 4
>    LE Create Connection (0x08|0x000d) status 0x00 ncmd 1
> 2012-05-07 11:21:44.752854 < HCI Command: LE Create Connection Cancel (0x08|0x000e) plen 0
> 2012-05-07 11:21:44.759475 > HCI Event: Command Complete (0x0e) plen 4
>    LE Create Connection Cancel (0x08|0x000e) ncmd 1
> 2012-05-07 11:21:44.764479 > HCI Event: LE Meta Event (0x3e) plen 19
>    LE Connection Complete
>      status 0x02 handle 0, role master
>      bdaddr 00:00:00:00:00:00 (Public)
>
> This causes problems in kernel since hci_conn is not properly removed and
> subsequent connections to this peer are not possible - since connection with
> peer 00:00:00:00:00:00 does not exist, it will be created and immediately
> removed (due to non-zero status code) leaving original hci_conn intact:
>
> [14898.739425] [6603] hci_connect: hci0 dst 00:22:D0:10:13:EE
> [14898.739429] [6603] hci_conn_add: hci0 dst 00:22:D0:10:13:EE
> [14898.739434] [6603] hci_conn_init_sysfs: conn ffff880079f03000
> [14898.739440] [6603] hci_send_cmd: hci0 opcode 0x200d plen 25
> [14898.739443] [6603] hci_send_cmd: skb len 28
> [14898.739487] [6603] hci_chan_create: hci0 conn ffff880079f03000
> ...
> [14938.860231] [55] hci_send_cmd: hci0 opcode 0x200e plen 0
> ...
> [14938.876427] [55] hci_le_conn_complete_evt: hci0 status 2
> [14938.876433] [55] hci_conn_add: hci0 dst 00:00:00:00:00:00
> [14938.876439] [55] hci_conn_init_sysfs: conn ffff88007aeff800
> [14938.876454] [55] hci_send_to_control: len 14
> [14938.876470] [55] l2cap_connect_cfm: hcon ffff88007aeff800 bdaddr 00:00:00:00:00:00 status 2
> [14938.876474] [55] hci_conn_del: hci0 conn ffff88007aeff800 handle 0
>
>
> I'm not sure if behaviour of BLE dongle is correct but I'd say yes since
> Bluetooth spec states in part E section 7.8.13 as follows:
> The LE Connection Complete event with the error code Unknown Connection
> Identifier (0x02) shall be sent after the Command Complete event for the
> LE_Create_Connection_Cancel command if the cancellation was successful.

PTS, CSR, and Atheros send the peer address properly. Broadcom and STE don't.
Indeed, the SPEC is not precise about this. Probably, the firmware
developers decided to send "00:00:00:00:00:00" to implement the white
list support easily.

BR,
Claudio.

2012-05-07 23:20:23

by Andrzej Kaczmarek

[permalink] [raw]
Subject: [PATCH] Bluetooth: Use hci_conn data to handle failed LE Connection Complete

This patch changes way LE Connection Complete event with error status are
handled by reusing peer information stored in hci_conn instead of these
returned in event packet which may not be always valid.

LE Connection Complete event for failed (cancelled) connection does not
include peer address on some Bluetooth chips and only status parameter is
filled. In such case appriopriate hci_conn is not removed and subsequent
connections to given peer won't be possible.

2012-05-07 11:21:39.133378 < HCI Command: LE Create Connection (0x08|0x000d) plen 25
bdaddr 00:22:D0:10:13:EE type 1
2012-05-07 11:21:39.138774 > HCI Event: Command Status (0x0f) plen 4
LE Create Connection (0x08|0x000d) status 0x00 ncmd 1
2012-05-07 11:21:44.752854 < HCI Command: LE Create Connection Cancel (0x08|0x000e) plen 0
2012-05-07 11:21:44.759475 > HCI Event: Command Complete (0x0e) plen 4
LE Create Connection Cancel (0x08|0x000e) ncmd 1
2012-05-07 11:21:44.764479 > HCI Event: LE Meta Event (0x3e) plen 19
LE Connection Complete
status 0x02 handle 0, role master
bdaddr 00:00:00:00:00:00 (Public)

[14898.739425] [6603] hci_connect: hci0 dst 00:22:D0:10:13:EE
[14898.739429] [6603] hci_conn_add: hci0 dst 00:22:D0:10:13:EE
[14898.739434] [6603] hci_conn_init_sysfs: conn ffff880079f03000
[14898.739440] [6603] hci_send_cmd: hci0 opcode 0x200d plen 25
[14898.739443] [6603] hci_send_cmd: skb len 28
[14898.739487] [6603] hci_chan_create: hci0 conn ffff880079f03000
...
[14938.860231] [55] hci_send_cmd: hci0 opcode 0x200e plen 0
...
[14938.876427] [55] hci_le_conn_complete_evt: hci0 status 2
[14938.876433] [55] hci_conn_add: hci0 dst 00:00:00:00:00:00
[14938.876439] [55] hci_conn_init_sysfs: conn ffff88007aeff800
[14938.876454] [55] hci_send_to_control: len 14
[14938.876470] [55] l2cap_connect_cfm: hcon ffff88007aeff800 bdaddr 00:00:00:00:00:00 status 2
[14938.876474] [55] hci_conn_del: hci0 conn ffff88007aeff800 handle 0

Signed-off-by: Andrzej Kaczmarek <[email protected]>
---
net/bluetooth/hci_event.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index b739baf..6ddb79e 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3300,6 +3300,19 @@ static inline void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff

hci_dev_lock(hdev);

+ if (ev->status) {
+ conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
+ if (!conn)
+ goto unlock;
+
+ mgmt_connect_failed(hdev, &conn->dst, conn->type,
+ conn->dst_type, ev->status);
+ hci_proto_connect_cfm(conn, ev->status);
+ conn->state = BT_CLOSED;
+ hci_conn_del(conn);
+ goto unlock;
+ }
+
conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &ev->bdaddr);
if (!conn) {
conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
@@ -3312,15 +3325,6 @@ static inline void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff
conn->dst_type = ev->bdaddr_type;
}

- if (ev->status) {
- mgmt_connect_failed(hdev, &ev->bdaddr, conn->type,
- conn->dst_type, ev->status);
- hci_proto_connect_cfm(conn, ev->status);
- conn->state = BT_CLOSED;
- hci_conn_del(conn);
- goto unlock;
- }
-
if (!test_and_set_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags))
mgmt_device_connected(hdev, &ev->bdaddr, conn->type,
conn->dst_type, 0, NULL, 0, NULL);
--
1.7.9.5