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]>
Acked-by: Andre Guedes <[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 4eefb7f..d9ec0e8 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3307,6 +3307,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);
@@ -3319,15 +3332,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
Hi Marcel,
On 25.05.2012 08:55, Marcel Holtmann wrote:
>> I was taking a look at Core spec change request document [1] and I found this:
>>
>> Erratum 4215, LE connection complete event missing exception
>> "... On failure, for this event, all other parameters are not valid."
>>
>> It clearly states this is an expected behavior and nullify those
>> parameters doesn't make the adapter broken.
>>
>> Thus, in case of failure, we should not rely on those parameters
>> (BD_ADDR included) in order to properly handle LE Connection Complete
>> Events.
>
> if this is so, then we should only do that and strictly enforce only one
> LE connection attempt at a time. Also then there is no need to bother
> with trying to check for the BD_ADDR later.
I created patch which does not allow 2nd outgoing LE connection attempt
and also reworded commit message for this one so it does state
explicitly that we should consider BDADDR from failed LE Connection
Complete as not valid and not use it.
Please take a look at my new patches (3 in total).
BR,
Andrzej
Hi Andre,
> >>>>>
> >>>>> 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);
> >>>>
> >>>>
> >>>> this change is wrong. We are now treating every single adapter as being
> >>>> broken. That is not acceptable.
> >>>
> >>>
> >>> Why do you think these adapters are broken? As I explained in cover
> >>> letter for v1, spec does not require peer address to be provided in
> >>> Connection Complete which is reasonable since we can only have one
> >>> pending connection request. Also as Claudio and Andre noticed such
> >>> behaviour could be to simplify whitelist implementation - in case of
> >>> connection request using whitelist it does not make sense to include
> >>> specific peer address in event.
> >>
> >>
> >> what has whitelist behavior to do with this event in the failure case?
> >
> >
> > Just a sidenote on why some vendors may want to omit BD_ADDR and it does not
> > make adapter broken. Not directly related to this scenario.
>
> I was taking a look at Core spec change request document [1] and I found this:
>
> Erratum 4215, LE connection complete event missing exception
> "... On failure, for this event, all other parameters are not valid."
>
> It clearly states this is an expected behavior and nullify those
> parameters doesn't make the adapter broken.
>
> Thus, in case of failure, we should not rely on those parameters
> (BD_ADDR included) in order to properly handle LE Connection Complete
> Events.
if this is so, then we should only do that and strictly enforce only one
LE connection attempt at a time. Also then there is no need to bother
with trying to check for the BD_ADDR later.
Regards
Marcel
Hi Andrzej/Marcel,
On Thu, May 17, 2012 at 5:05 AM, Andrzej Kaczmarek
<[email protected]> wrote:
> Hi Marcel,
>
>
> On 16.05.2012 23:48, Marcel Holtmann wrote:
>>>>>
>>>>> =A0 =A0 =A0 hci_dev_lock(hdev);
>>>>>
>>>>> + =A0 =A0 if (ev->status) {
>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 conn =3D hci_conn_hash_lookup_state(hdev, L=
E_LINK,
>>>>> BT_CONNECT);
>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 if (!conn)
>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto unlock;
>>>>> +
>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 mgmt_connect_failed(hdev,&conn->dst, conn->=
type,
>>>>>
>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 con=
n->dst_type, ev->status);
>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 hci_proto_connect_cfm(conn, ev->status);
>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 conn->state =3D BT_CLOSED;
>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 hci_conn_del(conn);
>>>>> + =A0 =A0 =A0 =A0 =A0 =A0 goto unlock;
>>>>> + =A0 =A0 }
>>>>> +
>>>>> =A0 =A0 =A0 conn =3D hci_conn_hash_lookup_ba(hdev, LE_LINK,&ev->bdadd=
r);
>>>>> =A0 =A0 =A0 if (!conn) {
>>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 conn =3D hci_conn_add(hdev, LE_LINK,&ev->=
bdaddr);
>>>>
>>>>
>>>> this change is wrong. We are now treating every single adapter as bein=
g
>>>> broken. That is not acceptable.
>>>
>>>
>>> Why do you think these adapters are broken? As I explained in cover
>>> letter for v1, spec does not require peer address to be provided in
>>> Connection Complete which is reasonable since we can only have one
>>> pending connection request. Also as Claudio and Andre noticed such
>>> behaviour could be to simplify whitelist implementation - in case of
>>> connection request using whitelist it does not make sense to include
>>> specific peer address in event.
>>
>>
>> what has whitelist behavior to do with this event in the failure case?
>
>
> Just a sidenote on why some vendors may want to omit BD_ADDR and it does =
not
> make adapter broken. Not directly related to this scenario.
I was taking a look at Core spec change request document [1] and I found th=
is:
Erratum 4215, LE connection complete event missing exception
"... On failure, for this event, all other parameters are not valid."
It clearly states this is an expected behavior and nullify those
parameters doesn't make the adapter broken.
Thus, in case of failure, we should not rely on those parameters
(BD_ADDR included) in order to properly handle LE Connection Complete
Events.
>>>> We should only add a tweak if the BD_ADDR parameter is BDADDR_ANY and
>>>> not as a general rule. In addition if we do this, we need to print a
>>>> warning to dmesg to make this known.
>>>
>>>
>>> Perhaps we can just add warning in case BD_ADDR is not BDADDR_ANY and
>>> we cannot find hci_conn for it - in such case most probably something
>>> went wrong.
>>
>>
>> What are the adapters from Broadcom, CSR, TI and ST are returning in a
>> failure case? Are they all omitting the BD_ADDR value?
>
>
> No, I noticed this on BCM and based on previous comments it seems that th=
is
> is what BCM and ST(E) are doing while CSR and TI return BD_ADDR. Except t=
hat
> previously I was using ST-E chip which did return BD_ADDR so this is not
> even consistent per manufacturer.
>
> As said before, omitted BD_ADDR seems to be fine from spec perspective so=
we
> should not require it to be present. Since we can have only one LE hci_co=
nn
> in BT_CONNECT state it's reasonable to use it here. This is already in
> patch.
>
> What could be indeed added to this patch is warning message in case BD_AD=
DR
> is not BDADDR_ANY and it does not match one stored in hci_conn (either
> adapter returns random BD_ADDR which is weird or something went wrong).
IMO, this warning message is not really necessary.
BR,
Andre
[1] - https://www.bluetooth.org/Technical/Specifications/enhancements.htm
(must be signed in Bluetooth SIG)
Hi Marcel,
On 16.05.2012 23:48, Marcel Holtmann wrote:
>>>> 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);
>>>
>>> this change is wrong. We are now treating every single adapter as being
>>> broken. That is not acceptable.
>>
>> Why do you think these adapters are broken? As I explained in cover
>> letter for v1, spec does not require peer address to be provided in
>> Connection Complete which is reasonable since we can only have one
>> pending connection request. Also as Claudio and Andre noticed such
>> behaviour could be to simplify whitelist implementation - in case of
>> connection request using whitelist it does not make sense to include
>> specific peer address in event.
>
> what has whitelist behavior to do with this event in the failure case?
Just a sidenote on why some vendors may want to omit BD_ADDR and it does
not make adapter broken. Not directly related to this scenario.
>>> We should only add a tweak if the BD_ADDR parameter is BDADDR_ANY and
>>> not as a general rule. In addition if we do this, we need to print a
>>> warning to dmesg to make this known.
>>
>> Perhaps we can just add warning in case BD_ADDR is not BDADDR_ANY and
>> we cannot find hci_conn for it - in such case most probably something
>> went wrong.
>
> What are the adapters from Broadcom, CSR, TI and ST are returning in a
> failure case? Are they all omitting the BD_ADDR value?
No, I noticed this on BCM and based on previous comments it seems that
this is what BCM and ST(E) are doing while CSR and TI return BD_ADDR.
Except that previously I was using ST-E chip which did return BD_ADDR so
this is not even consistent per manufacturer.
As said before, omitted BD_ADDR seems to be fine from spec perspective
so we should not require it to be present. Since we can have only one LE
hci_conn in BT_CONNECT state it's reasonable to use it here. This is
already in patch.
What could be indeed added to this patch is warning message in case
BD_ADDR is not BDADDR_ANY and it does not match one stored in hci_conn
(either adapter returns random BD_ADDR which is weird or something went
wrong).
BR,
Andrzej
Hi Andrzej,
> >> 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);
> >
> > this change is wrong. We are now treating every single adapter as being
> > broken. That is not acceptable.
>
> Why do you think these adapters are broken? As I explained in cover
> letter for v1, spec does not require peer address to be provided in
> Connection Complete which is reasonable since we can only have one
> pending connection request. Also as Claudio and Andre noticed such
> behaviour could be to simplify whitelist implementation - in case of
> connection request using whitelist it does not make sense to include
> specific peer address in event.
what has whitelist behavior to do with this event in the failure case?
> > We should only add a tweak if the BD_ADDR parameter is BDADDR_ANY and
> > not as a general rule. In addition if we do this, we need to print a
> > warning to dmesg to make this known.
>
> Perhaps we can just add warning in case BD_ADDR is not BDADDR_ANY and
> we cannot find hci_conn for it - in such case most probably something
> went wrong.
What are the adapters from Broadcom, CSR, TI and ST are returning in a
failure case? Are they all omitting the BD_ADDR value?
Regards
Marcel
Hi Marcel,
2012/5/16 Marcel Holtmann <[email protected]>:
>> =A0 =A0 =A0 hci_dev_lock(hdev);
>>
>> + =A0 =A0 if (ev->status) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 conn =3D hci_conn_hash_lookup_state(hdev, LE_L=
INK, BT_CONNECT);
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (!conn)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto unlock;
>> +
>> + =A0 =A0 =A0 =A0 =A0 =A0 mgmt_connect_failed(hdev, &conn->dst, conn->ty=
pe,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 conn->=
dst_type, ev->status);
>> + =A0 =A0 =A0 =A0 =A0 =A0 hci_proto_connect_cfm(conn, ev->status);
>> + =A0 =A0 =A0 =A0 =A0 =A0 conn->state =3D BT_CLOSED;
>> + =A0 =A0 =A0 =A0 =A0 =A0 hci_conn_del(conn);
>> + =A0 =A0 =A0 =A0 =A0 =A0 goto unlock;
>> + =A0 =A0 }
>> +
>> =A0 =A0 =A0 conn =3D hci_conn_hash_lookup_ba(hdev, LE_LINK, &ev->bdaddr)=
;
>> =A0 =A0 =A0 if (!conn) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 conn =3D hci_conn_add(hdev, LE_LINK, &ev->bd=
addr);
>
> this change is wrong. We are now treating every single adapter as being
> broken. That is not acceptable.
Why do you think these adapters are broken? As I explained in cover
letter for v1, spec does not require peer address to be provided in
Connection Complete which is reasonable since we can only have one
pending connection request. Also as Claudio and Andre noticed such
behaviour could be to simplify whitelist implementation - in case of
connection request using whitelist it does not make sense to include
specific peer address in event.
> We should only add a tweak if the BD_ADDR parameter is BDADDR_ANY and
> not as a general rule. In addition if we do this, we need to print a
> warning to dmesg to make this known.
Perhaps we can just add warning in case BD_ADDR is not BDADDR_ANY and
we cannot find hci_conn for it - in such case most probably something
went wrong.
BR,
Andrzej
Hi Andrzej,
> 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]>
> Acked-by: Andre Guedes <[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 4eefb7f..d9ec0e8 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3307,6 +3307,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);
this change is wrong. We are now treating every single adapter as being
broken. That is not acceptable.
We should only add a tweak if the BD_ADDR parameter is BDADDR_ANY and
not as a general rule. In addition if we do this, we need to print a
warning to dmesg to make this known.
I also like to clearly state in the commit message which manufactures
have these broken adapters.
Regards
Marcel