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

[Marcel Holtmann <marcel@holtmann.org>]

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