Re: [PATCH v2] Bluetooth: Use hci_conn data to handle failed LE Connection Complete

[Andrzej Kaczmarek <andrzej.kaczmarek@tieto.com>]

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