Hi Marcel,
>> @@ -356,6 +356,9 @@ static void hci_conn_timeout(struct work_struct *work)
>> conn->state = BT_CLOSED;
>> break;
>> }
>> +
>> + if (test_and_clear_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags))
>> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
>
> do we really need this one? The hci_conn_timeout should trigger when the connection establishment has a timeout and that is really non of the concerns here. And in case we hit an idle disconnect timeout, we are still ending up in hci_conn_del eventually, right? If not, I am having the slight feeling that you might have uncovered a bug that we should fix.
It's probably just lack of familiarity with the code, but it wasn't
all that clear to me that hci_conn_del is called after
hci_conn_timeout kicks in. I removed it.
>
>>
>> err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
>> }
>> @@ -2748,12 +2758,12 @@ static int unpair_device(struct sock *sk, struct hci_dev *hdev, void *data,
>> }
>>
>> if (cp->disconnect) {
>> + /* Only lookup the connection in the BR/EDR since the
>> + * LE connection was already looked up earlier.
>> + */
>> if (cp->addr.type == BDADDR_BREDR)
>> conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
>> &cp->addr.bdaddr);
>> - else
>> - conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
>> - &cp->addr.bdaddr);
>> } else {
>> conn = NULL;
>> }
>
> I know that Johan told you to do it this way, but seeing the patch now makes me a bit uneasy. The usage of uninitilized_var in this case can lead to errors in the future. The logic that the compiler is wrong here is not obvious. Which means a week from now all of us have forgotten why we did it.
>
> So here is what I would do to make this easier.
>
> if (cp->addr.type == BDADDR_BREDR) {
> if (cp->disconnect) {
> conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,
> &cp->addr.bdaddr);
> else
> conn = NULL;
>
> err = hci_remove_link_key(hdev, &cp->addr.bdaddr);
> } else {
> conn = hci_conn_hash_lookup_ba(hdev, LE_LINK,
> &cp->addr.bdaddr);
> if (conn) {
> set_bit(HCI_CONN_PARAM_REMOVAL_PEND, &conn->flags);
>
> if (!cp->disconnect)
> conn = NULL;
> }
>
> ...
>
> hci_remove_irk(hdev, &cp->addr.bdaddr, addr_type);
>
> err = hci_remove_ltk(hdev, &cp->addr.bdaddr, addr_type);
> }
>
> Add some minor comments on why we set conn = NULL and this should be a simpler block. Mainly because the difference between BR/EDR and LE are contained in a single spot and not spread out over two places.
>
> This already takes into account my comment from above to just set the PARAM_REMOVAL flag no matter if we actually be asked to disconnect or not.
Yep, I agree, what you propose is way cleaner. Thanks.
V4 takes care of your remarks.
--
Alfonso Acosta
Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
