LinuxLists.cc - [PATCH v5 1/6] Bluetooth: add HCI_LE_CONNECTING hci

2015-07-29 09:21:49

Subject: [PATCH v5 1/6] Bluetooth: add HCI_LE_CONNECTING hci_dev flags

This patch adds HCI_LE_CONNECTING flag to hci_dev flags, that will be set
when connect attempt is pending. It also uses this flag instead of conn
hash lookup where appropriate to check wether connect is in progress.

Signed-off-by: Jakub Pawlowski <[email protected]>
---
include/net/bluetooth/hci.h | 1 +
net/bluetooth/hci_conn.c | 3 +--
net/bluetooth/hci_event.c | 4 ++++
net/bluetooth/hci_request.c | 6 ++----
net/bluetooth/mgmt.c | 2 +-
5 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 7ca6690..4772ff8 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -240,6 +240,7 @@ enum {
HCI_DUT_MODE,
HCI_FORCE_BREDR_SMP,
HCI_FORCE_STATIC_ADDR,
+ HCI_LE_CONNECTING,

__HCI_NUM_FLAGS,
};
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 2c48bf0..1ba8240 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -759,8 +759,7 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
/* Since the controller supports only one LE connection attempt at a
* time, we return -EBUSY if there is any connection attempt running.
*/
- conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
- if (conn)
+ if (hci_dev_test_flag(hdev, HCI_LE_CONNECTING))
return ERR_PTR(-EBUSY);

/* When given an identity address with existing identity
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 32363c2..18fccf8 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2005,6 +2005,8 @@ static void hci_cs_le_create_conn(struct hci_dev *hdev, u8 status)

hci_dev_lock(hdev);

+ hci_dev_set_flag(hdev, HCI_LE_CONNECTING);
+
conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &cp->peer_addr);
if (!conn)
goto unlock;
@@ -4535,6 +4537,8 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)

hci_dev_lock(hdev);

+ hci_dev_clear_flag(hdev, HCI_LE_CONNECTING);
+
/* All controllers implicitly stop advertising in the event of a
* connection, so ensure that the state bit is cleared.
*/
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index d6025d6..daebb75 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -317,7 +317,7 @@ static void set_random_addr(struct hci_request *req, bdaddr_t *rpa)
* address be updated at the next cycle.
*/
if (hci_dev_test_flag(hdev, HCI_LE_ADV) ||
- hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT)) {
+ hci_dev_test_flag(hdev, HCI_LE_CONNECTING)) {
BT_DBG("Deferring random address update");
hci_dev_set_flag(hdev, HCI_RPA_EXPIRED);
return;
@@ -479,7 +479,6 @@ void hci_update_page_scan(struct hci_dev *hdev)
void __hci_update_background_scan(struct hci_request *req)
{
struct hci_dev *hdev = req->hdev;
- struct hci_conn *conn;

if (!test_bit(HCI_UP, &hdev->flags) ||
test_bit(HCI_INIT, &hdev->flags) ||
@@ -529,8 +528,7 @@ void __hci_update_background_scan(struct hci_request *req)
* since some controllers are not able to scan and connect at
* the same time.
*/
- conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
- if (conn)
+ if (hci_dev_test_flag(hdev, HCI_LE_CONNECTING))
return;

/* If controller is currently scanning, we stop it to ensure we
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 7ab1915..c35d671 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4210,7 +4210,7 @@ static bool trigger_le_scan(struct hci_request *req, u16 interval, u8 *status)
/* Don't let discovery abort an outgoing connection attempt
* that's using directed advertising.
*/
- if (hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT)) {
+ if (hci_dev_test_flag(hdev, HCI_LE_CONNECTING)) {
*status = MGMT_STATUS_REJECTED;
return false;
}
--
2.1.4

2015-07-29 16:58:23

by Marcel Holtmann

[permalink] [raw]

Subject: Re: [PATCH v5 3/6] Bluetooth: add hci_connect_le_scan

Hi Jakub,

>>>>> Currently, when trying to connect to already paired device that just
>>>>> rotated its RPA MAC address, old address would be used and connection
>>>>> would fail. In order to fix that, kernel must scan and receive
>>>>> advertisement with fresh RPA before connecting.
>>>>>
>>>>> This patch adds hci_connect_le_scan with dependencies, new method that
>>>>> will be used to connect to remote LE devices. Instead of just sending
>>>>> connect request, it adds a device to whitelist. Later patches will make
>>>>> use of this whitelist to send conenct request when advertisement is
>>>>> received, and properly handle timeouts.
>>>>>
>>>>> Signed-off-by: Jakub Pawlowski <[email protected]>
>>>>> ---
>>>>> include/net/bluetooth/hci_core.h | 5 ++
>>>>> net/bluetooth/hci_conn.c | 170 +++++++++++++++++++++++++++++++++++++++
>>>>> net/bluetooth/hci_core.c | 32 ++++++++
>>>>> 3 files changed, 207 insertions(+)
>>>>>
>>>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>>>> index c8d2b5a..5d19794 100644
>>>>> --- a/include/net/bluetooth/hci_core.h
>>>>> +++ b/include/net/bluetooth/hci_core.h
>>>>> @@ -826,6 +826,9 @@ void hci_chan_del(struct hci_chan *chan);
>>>>> void hci_chan_list_flush(struct hci_conn *conn);
>>>>> struct hci_chan *hci_chan_lookup_handle(struct hci_dev *hdev, __u16 handle);
>>>>>
>>>>> +struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst,
>>>>> + u8 dst_type, u8 sec_level,
>>>>> + u16 conn_timeout, u8 role);
>>>>> struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
>>>>> u8 dst_type, u8 sec_level, u16 conn_timeout,
>>>>> u8 role);
>>>>> @@ -991,6 +994,8 @@ void hci_conn_params_clear_disabled(struct hci_dev *hdev);
>>>>> struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list,
>>>>> bdaddr_t *addr,
>>>>> u8 addr_type);
>>>>> +struct hci_conn_params *hci_pend_le_explicit_connect_lookup(
>>>>> + struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type);
>>>>
>>>> this is kinda violating the coding style. I wonder if we can shorten the function name or just go over 80 characters this time.
>>>
>>> will rename it to hci_explicit_connect_lookup
>>>>
>>>>> void hci_uuids_clear(struct hci_dev *hdev);
>>>>>
>>>>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>>>>> index 1ba8240..2a13214 100644
>>>>> --- a/net/bluetooth/hci_conn.c
>>>>> +++ b/net/bluetooth/hci_conn.c
>>>>> @@ -64,6 +64,36 @@ static void hci_le_create_connection_cancel(struct hci_conn *conn)
>>>>> hci_send_cmd(conn->hdev, HCI_OP_LE_CREATE_CONN_CANCEL, 0, NULL);
>>>>> }
>>>>>
>>>>> +/* This function requires the caller holds hdev->lock */
>>>>> +static void hci_connect_le_scan_cleanup(struct hci_conn *conn)
>>>>> +{
>>>>> + struct hci_conn_params *params;
>>>>> +
>>>>> + params = hci_pend_le_explicit_connect_lookup(conn->hdev, &conn->dst,
>>>>> + conn->dst_type);
>>>>> + if (!params)
>>>>> + return;
>>>>> +
>>>>> + /* The connection attempt was doing scan for new RPA, and is
>>>>> + * in scan phase. If params are not associated with any other
>>>>> + * autoconnect action, remove them completely. If they are, just unmark
>>>>> + * them as awaiting for connection, by clearing explicit_connect field.
>>>>> + */
>>>>> + if (params->auto_connect == HCI_AUTO_CONN_EXPLICIT)
>>>>> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
>>>>> + else
>>>>> + params->explicit_connect = false;
>>>>> +}
>>>>
>>>> What happens if we have two explicit connection requests here. I mean we can have two L2CAP socket trying to connect to two peripherals at the same time.
>>>>
>>>
>>> So right now if we have two sockets connecting to two different
>>> peripherals, you'll get same behavior as before, that is first request
>>> will succeed, second will fail (if they're triggered at same time and
>>> connection is not established before second is attempted). I'm willing
>>> to further improve this behavior in future.
>>
>>
>> essentially we just need to add both connect requests to the auto-connect list and it will sort itself out. Which means however we need to make sure we track this correct.
>>
>
> No, If you try again both sockets would use same conn object. call to
> connect on each socket will result in hci_connect_le_scan. First call
> to this method will create conn object, second call will re-use the
> old object for second socket. But now I found a bug - if one of
> sockets get closed, second will also get closed, because I don't keep
> count of sockets trying to connect. I'll fix that by changing
> explicit_connect from boolean to integer and increment it on each
> connect attempt.

that is for two L2CAP connection to the same device. That is something that we should consider as well. However isn't the hci_conn reference counting already taking care of that.

I was also worrying about one L2CAP connection to device a and another to device b. Both happening at the same time. Meaning whichever scanning sees first will connect first, but they will be connected both. It should work the same way as if you have two Add Device auto-connect devices and they are both available. They will both connect after each other.

Regards

Marcel

2015-07-29 16:36:59

by Jakub Pawlowski

[permalink] [raw]

Subject: Re: [PATCH v5 3/6] Bluetooth: add hci_connect_le_scan

On Wed, Jul 29, 2015 at 5:37 PM, Marcel Holtmann <[email protected]> wrote:
> Hi Jakub,
>
>>>> Currently, when trying to connect to already paired device that just
>>>> rotated its RPA MAC address, old address would be used and connection
>>>> would fail. In order to fix that, kernel must scan and receive
>>>> advertisement with fresh RPA before connecting.
>>>>
>>>> This patch adds hci_connect_le_scan with dependencies, new method that
>>>> will be used to connect to remote LE devices. Instead of just sending
>>>> connect request, it adds a device to whitelist. Later patches will make
>>>> use of this whitelist to send conenct request when advertisement is
>>>> received, and properly handle timeouts.
>>>>
>>>> Signed-off-by: Jakub Pawlowski <[email protected]>
>>>> ---
>>>> include/net/bluetooth/hci_core.h | 5 ++
>>>> net/bluetooth/hci_conn.c | 170 +++++++++++++++++++++++++++++++++++++++
>>>> net/bluetooth/hci_core.c | 32 ++++++++
>>>> 3 files changed, 207 insertions(+)
>>>>
>>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>>> index c8d2b5a..5d19794 100644
>>>> --- a/include/net/bluetooth/hci_core.h
>>>> +++ b/include/net/bluetooth/hci_core.h
>>>> @@ -826,6 +826,9 @@ void hci_chan_del(struct hci_chan *chan);
>>>> void hci_chan_list_flush(struct hci_conn *conn);
>>>> struct hci_chan *hci_chan_lookup_handle(struct hci_dev *hdev, __u16 handle);
>>>>
>>>> +struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst,
>>>> + u8 dst_type, u8 sec_level,
>>>> + u16 conn_timeout, u8 role);
>>>> struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
>>>> u8 dst_type, u8 sec_level, u16 conn_timeout,
>>>> u8 role);
>>>> @@ -991,6 +994,8 @@ void hci_conn_params_clear_disabled(struct hci_dev *hdev);
>>>> struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list,
>>>> bdaddr_t *addr,
>>>> u8 addr_type);
>>>> +struct hci_conn_params *hci_pend_le_explicit_connect_lookup(
>>>> + struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type);
>>>
>>> this is kinda violating the coding style. I wonder if we can shorten the function name or just go over 80 characters this time.
>>
>> will rename it to hci_explicit_connect_lookup
>>>
>>>> void hci_uuids_clear(struct hci_dev *hdev);
>>>>
>>>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>>>> index 1ba8240..2a13214 100644
>>>> --- a/net/bluetooth/hci_conn.c
>>>> +++ b/net/bluetooth/hci_conn.c
>>>> @@ -64,6 +64,36 @@ static void hci_le_create_connection_cancel(struct hci_conn *conn)
>>>> hci_send_cmd(conn->hdev, HCI_OP_LE_CREATE_CONN_CANCEL, 0, NULL);
>>>> }
>>>>
>>>> +/* This function requires the caller holds hdev->lock */
>>>> +static void hci_connect_le_scan_cleanup(struct hci_conn *conn)
>>>> +{
>>>> + struct hci_conn_params *params;
>>>> +
>>>> + params = hci_pend_le_explicit_connect_lookup(conn->hdev, &conn->dst,
>>>> + conn->dst_type);
>>>> + if (!params)
>>>> + return;
>>>> +
>>>> + /* The connection attempt was doing scan for new RPA, and is
>>>> + * in scan phase. If params are not associated with any other
>>>> + * autoconnect action, remove them completely. If they are, just unmark
>>>> + * them as awaiting for connection, by clearing explicit_connect field.
>>>> + */
>>>> + if (params->auto_connect == HCI_AUTO_CONN_EXPLICIT)
>>>> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
>>>> + else
>>>> + params->explicit_connect = false;
>>>> +}
>>>
>>> What happens if we have two explicit connection requests here. I mean we can have two L2CAP socket trying to connect to two peripherals at the same time.
>>>
>>
>> So right now if we have two sockets connecting to two different
>> peripherals, you'll get same behavior as before, that is first request
>> will succeed, second will fail (if they're triggered at same time and
>> connection is not established before second is attempted). I'm willing
>> to further improve this behavior in future.
>
>
> essentially we just need to add both connect requests to the auto-connect list and it will sort itself out. Which means however we need to make sure we track this correct.
>
> Regards
>
> Marcel
>

No, If you try again both sockets would use same conn object. call to
connect on each socket will result in hci_connect_le_scan. First call
to this method will create conn object, second call will re-use the
old object for second socket. But now I found a bug - if one of
sockets get closed, second will also get closed, because I don't keep
count of sockets trying to connect. I'll fix that by changing
explicit_connect from boolean to integer and increment it on each
connect attempt.

Plus I'll add a test case for that.

2015-07-29 15:37:56

by Marcel Holtmann

[permalink] [raw]

Subject: Re: [PATCH v5 3/6] Bluetooth: add hci_connect_le_scan

Hi Jakub,

>>> Currently, when trying to connect to already paired device that just
>>> rotated its RPA MAC address, old address would be used and connection
>>> would fail. In order to fix that, kernel must scan and receive
>>> advertisement with fresh RPA before connecting.
>>>
>>> This patch adds hci_connect_le_scan with dependencies, new method that
>>> will be used to connect to remote LE devices. Instead of just sending
>>> connect request, it adds a device to whitelist. Later patches will make
>>> use of this whitelist to send conenct request when advertisement is
>>> received, and properly handle timeouts.
>>>
>>> Signed-off-by: Jakub Pawlowski <[email protected]>
>>> ---
>>> include/net/bluetooth/hci_core.h | 5 ++
>>> net/bluetooth/hci_conn.c | 170 +++++++++++++++++++++++++++++++++++++++
>>> net/bluetooth/hci_core.c | 32 ++++++++
>>> 3 files changed, 207 insertions(+)
>>>
>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>> index c8d2b5a..5d19794 100644
>>> --- a/include/net/bluetooth/hci_core.h
>>> +++ b/include/net/bluetooth/hci_core.h
>>> @@ -826,6 +826,9 @@ void hci_chan_del(struct hci_chan *chan);
>>> void hci_chan_list_flush(struct hci_conn *conn);
>>> struct hci_chan *hci_chan_lookup_handle(struct hci_dev *hdev, __u16 handle);
>>>
>>> +struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst,
>>> + u8 dst_type, u8 sec_level,
>>> + u16 conn_timeout, u8 role);
>>> struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
>>> u8 dst_type, u8 sec_level, u16 conn_timeout,
>>> u8 role);
>>> @@ -991,6 +994,8 @@ void hci_conn_params_clear_disabled(struct hci_dev *hdev);
>>> struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list,
>>> bdaddr_t *addr,
>>> u8 addr_type);
>>> +struct hci_conn_params *hci_pend_le_explicit_connect_lookup(
>>> + struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type);
>>
>> this is kinda violating the coding style. I wonder if we can shorten the function name or just go over 80 characters this time.
>
> will rename it to hci_explicit_connect_lookup
>>
>>> void hci_uuids_clear(struct hci_dev *hdev);
>>>
>>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>>> index 1ba8240..2a13214 100644
>>> --- a/net/bluetooth/hci_conn.c
>>> +++ b/net/bluetooth/hci_conn.c
>>> @@ -64,6 +64,36 @@ static void hci_le_create_connection_cancel(struct hci_conn *conn)
>>> hci_send_cmd(conn->hdev, HCI_OP_LE_CREATE_CONN_CANCEL, 0, NULL);
>>> }
>>>
>>> +/* This function requires the caller holds hdev->lock */
>>> +static void hci_connect_le_scan_cleanup(struct hci_conn *conn)
>>> +{
>>> + struct hci_conn_params *params;
>>> +
>>> + params = hci_pend_le_explicit_connect_lookup(conn->hdev, &conn->dst,
>>> + conn->dst_type);
>>> + if (!params)
>>> + return;
>>> +
>>> + /* The connection attempt was doing scan for new RPA, and is
>>> + * in scan phase. If params are not associated with any other
>>> + * autoconnect action, remove them completely. If they are, just unmark
>>> + * them as awaiting for connection, by clearing explicit_connect field.
>>> + */
>>> + if (params->auto_connect == HCI_AUTO_CONN_EXPLICIT)
>>> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
>>> + else
>>> + params->explicit_connect = false;
>>> +}
>>
>> What happens if we have two explicit connection requests here. I mean we can have two L2CAP socket trying to connect to two peripherals at the same time.
>>
>
> So right now if we have two sockets connecting to two different
> peripherals, you'll get same behavior as before, that is first request
> will succeed, second will fail (if they're triggered at same time and
> connection is not established before second is attempted). I'm willing
> to further improve this behavior in future.

essentially we just need to add both connect requests to the auto-connect list and it will sort itself out. Which means however we need to make sure we track this correct.

Regards

Marcel

2015-07-29 14:09:49

by Jakub Pawlowski

[permalink] [raw]

Subject: Re: [PATCH v5 3/6] Bluetooth: add hci_connect_le_scan

On Wed, Jul 29, 2015 at 4:02 PM, Marcel Holtmann <[email protected]> wrote:
> Hi Jakub,
>
>> Currently, when trying to connect to already paired device that just
>> rotated its RPA MAC address, old address would be used and connection
>> would fail. In order to fix that, kernel must scan and receive
>> advertisement with fresh RPA before connecting.
>>
>> This patch adds hci_connect_le_scan with dependencies, new method that
>> will be used to connect to remote LE devices. Instead of just sending
>> connect request, it adds a device to whitelist. Later patches will make
>> use of this whitelist to send conenct request when advertisement is
>> received, and properly handle timeouts.
>>
>> Signed-off-by: Jakub Pawlowski <[email protected]>
>> ---
>> include/net/bluetooth/hci_core.h | 5 ++
>> net/bluetooth/hci_conn.c | 170 +++++++++++++++++++++++++++++++++++++++
>> net/bluetooth/hci_core.c | 32 ++++++++
>> 3 files changed, 207 insertions(+)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index c8d2b5a..5d19794 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -826,6 +826,9 @@ void hci_chan_del(struct hci_chan *chan);
>> void hci_chan_list_flush(struct hci_conn *conn);
>> struct hci_chan *hci_chan_lookup_handle(struct hci_dev *hdev, __u16 handle);
>>
>> +struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst,
>> + u8 dst_type, u8 sec_level,
>> + u16 conn_timeout, u8 role);
>> struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
>> u8 dst_type, u8 sec_level, u16 conn_timeout,
>> u8 role);
>> @@ -991,6 +994,8 @@ void hci_conn_params_clear_disabled(struct hci_dev *hdev);
>> struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list,
>> bdaddr_t *addr,
>> u8 addr_type);
>> +struct hci_conn_params *hci_pend_le_explicit_connect_lookup(
>> + struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type);
>
> this is kinda violating the coding style. I wonder if we can shorten the function name or just go over 80 characters this time.

will rename it to hci_explicit_connect_lookup
>
>> void hci_uuids_clear(struct hci_dev *hdev);
>>
>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>> index 1ba8240..2a13214 100644
>> --- a/net/bluetooth/hci_conn.c
>> +++ b/net/bluetooth/hci_conn.c
>> @@ -64,6 +64,36 @@ static void hci_le_create_connection_cancel(struct hci_conn *conn)
>> hci_send_cmd(conn->hdev, HCI_OP_LE_CREATE_CONN_CANCEL, 0, NULL);
>> }
>>
>> +/* This function requires the caller holds hdev->lock */
>> +static void hci_connect_le_scan_cleanup(struct hci_conn *conn)
>> +{
>> + struct hci_conn_params *params;
>> +
>> + params = hci_pend_le_explicit_connect_lookup(conn->hdev, &conn->dst,
>> + conn->dst_type);
>> + if (!params)
>> + return;
>> +
>> + /* The connection attempt was doing scan for new RPA, and is
>> + * in scan phase. If params are not associated with any other
>> + * autoconnect action, remove them completely. If they are, just unmark
>> + * them as awaiting for connection, by clearing explicit_connect field.
>> + */
>> + if (params->auto_connect == HCI_AUTO_CONN_EXPLICIT)
>> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
>> + else
>> + params->explicit_connect = false;
>> +}
>
> What happens if we have two explicit connection requests here. I mean we can have two L2CAP socket trying to connect to two peripherals at the same time.
>

So right now if we have two sockets connecting to two different
peripherals, you'll get same behavior as before, that is first request
will succeed, second will fail (if they're triggered at same time and
connection is not established before second is attempted). I'm willing
to further improve this behavior in future.

>> +
>> +/* This function requires the caller holds hdev->lock */
>> +static void hci_connect_le_scan_remove(struct hci_conn *conn)
>> +{
>> + hci_connect_le_scan_cleanup(conn);
>> +
>> + hci_conn_hash_del(conn->hdev, conn);
>> + hci_update_background_scan(conn->hdev);
>> +}
>> +
>> static void hci_acl_create_connection(struct hci_conn *conn)
>> {
>> struct hci_dev *hdev = conn->hdev;
>> @@ -858,6 +888,146 @@ done:
>> return conn;
>> }
>>
>> +static void hci_connect_le_scan_complete(struct hci_dev *hdev, u8 status,
>> + u16 opcode)
>> +{
>> + struct hci_conn *conn;
>> +
>> + if (status == 0)
>> + return;
>> +
>
> We prefer if (!status) these days.
>
>> + BT_ERR("Failed to add device to auto conn whitelist: status 0x%2.2x",
>> + status);
>> +
>> + hci_dev_lock(hdev);
>> +
>> + conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
>> + if (!conn)
>> + goto done;
>> +
>> + hci_le_conn_failed(conn, status);
>> +
>> +done:
>
> Remove the label here. Just move hci_le_conn_failed under if (conn) check.
>
>> + hci_dev_unlock(hdev);
>> +}
>> +
>> +static bool is_connected(struct hci_dev *hdev, bdaddr_t *addr, u8 type)
>> +{
>> + struct hci_conn *conn;
>> +
>> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, addr);
>> + if (!conn)
>> + return false;
>> +
>> + if (conn->dst_type != type)
>> + return false;
>> +
>> + if (conn->state != BT_CONNECTED)
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> +/* This function requires the caller holds hdev->lock */
>> +int hci_explicit_conn_params_set(struct hci_request *req, bdaddr_t *addr,
>> + u8 addr_type)
>> +{
>> + struct hci_dev *hdev = req->hdev;
>> + struct hci_conn_params *params;
>> +
>> + if (is_connected(hdev, addr, addr_type))
>> + return -EISCONN;
>> +
>> + params = hci_conn_params_add(hdev, addr, addr_type);
>> + if (!params)
>> + return -EIO;
>> +
>> + /* If we created new params, or exiting params were marked as disabled,
>> + * mark them to be used just once to connect.
>> + */
>> + if (params->auto_connect == HCI_AUTO_CONN_DISABLED) {
>> + params->auto_connect = HCI_AUTO_CONN_EXPLICIT;
>> + list_del_init(&params->action);
>> + list_add(&params->action, &hdev->pend_le_conns);
>> + }
>> +
>> + params->explicit_connect = true;
>> + __hci_update_background_scan(req);
>> +
>> + BT_DBG("addr %pMR (type %u) auto_connect %u", addr, addr_type,
>> + params->auto_connect);
>> +
>> + return 0;
>> +}
>> +
>> +/* This function requires the caller holds hdev->lock */
>> +struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst,
>> + u8 dst_type, u8 sec_level,
>> + u16 conn_timeout, u8 role)
>> +{
>> + struct hci_conn *conn;
>> + struct hci_request req;
>> + int err;
>> +
>> + /* Let's make sure that le is enabled.*/
>> + if (!hci_dev_test_flag(hdev, HCI_LE_ENABLED)) {
>> + if (lmp_le_capable(hdev))
>> + return ERR_PTR(-ECONNREFUSED);
>> +
>> + return ERR_PTR(-EOPNOTSUPP);
>> + }
>> +
>> + /* Some devices send ATT messages as soon as the physical link is
>> + * established. To be able to handle these ATT messages, the user-
>> + * space first establishes the connection and then starts the pairing
>> + * process.
>> + *
>> + * So if a hci_conn object already exists for the following connection
>> + * attempt, we simply update pending_sec_level and auth_type fields
>> + * and return the object found.
>> + */
>> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
>> + if (conn) {
>> + if (conn->pending_sec_level < sec_level)
>> + conn->pending_sec_level = sec_level;
>> + goto done;
>> + }
>> +
>> + BT_DBG("requesting refresh of dst_addr.");
>> +
>> + /* We support only one connection attempt at a time. */
>> + conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
>> + if (conn)
>> + return ERR_PTR(-EBUSY);
>> +
>> + conn = hci_conn_add(hdev, LE_LINK, dst, role);
>> + if (!conn)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + hci_req_init(&req, hdev);
>> +
>> + if (hci_explicit_conn_params_set(&req, dst, dst_type) < 0)
>> + return ERR_PTR(-EBUSY);
>> +
>> + conn->state = BT_CONNECT;
>> + set_bit(HCI_CONN_SCANNING, &conn->flags);
>> +
>> + err = hci_req_run(&req, hci_connect_le_scan_complete);
>> + if (err && err != -ENODATA) {
>> + hci_conn_del(conn);
>> + return ERR_PTR(err);
>> + }
>> +
>> + conn->dst_type = dst_type;
>> + conn->sec_level = BT_SECURITY_LOW;
>> + conn->pending_sec_level = sec_level;
>> + conn->conn_timeout = conn_timeout;
>> +
>> +done:
>> + hci_conn_hold(conn);
>> + return conn;
>> +}
>> +
>> struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
>> u8 sec_level, u8 auth_type)
>> {
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index bc43b64..e322aac 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -2848,6 +2848,29 @@ struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list,
>> }
>>
>> /* This function requires the caller holds hdev->lock */
>> +struct hci_conn_params *hci_pend_le_explicit_connect_lookup(
>> + struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
>> +{
>> + struct hci_conn_params *param;
>> +
>> + list_for_each_entry(param, &hdev->pend_le_conns, action) {
>> + if (bacmp(&param->addr, addr) == 0 &&
>> + param->addr_type == addr_type &&
>> + param->explicit_connect)
>> + return param;
>> + }
>> +
>> + list_for_each_entry(param, &hdev->pend_le_reports, action) {
>> + if (bacmp(&param->addr, addr) == 0 &&
>> + param->addr_type == addr_type &&
>> + param->explicit_connect)
>> + return param;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +/* This function requires the caller holds hdev->lock */
>> struct hci_conn_params *hci_conn_params_add(struct hci_dev *hdev,
>> bdaddr_t *addr, u8 addr_type)
>> {
>> @@ -2916,6 +2939,15 @@ void hci_conn_params_clear_disabled(struct hci_dev *hdev)
>> list_for_each_entry_safe(params, tmp, &hdev->le_conn_params, list) {
>> if (params->auto_connect != HCI_AUTO_CONN_DISABLED)
>> continue;
>> +
>> + /* If trying to estabilish one time connection to disabled
>> + * device, leave the params, but mark them as just once.
>> + */
>> + if (params->explicit_connect) {
>> + params->auto_connect = HCI_AUTO_CONN_EXPLICIT;
>> + continue;
>> + }
>> +
>> list_del(&params->list);
>> kfree(params);
>> }
>
> Regards
>
> Marcel
>

2015-07-29 14:02:42

by Marcel Holtmann

[permalink] [raw]

Subject: Re: [PATCH v5 3/6] Bluetooth: add hci_connect_le_scan

Hi Jakub,

> Currently, when trying to connect to already paired device that just
> rotated its RPA MAC address, old address would be used and connection
> would fail. In order to fix that, kernel must scan and receive
> advertisement with fresh RPA before connecting.
>
> This patch adds hci_connect_le_scan with dependencies, new method that
> will be used to connect to remote LE devices. Instead of just sending
> connect request, it adds a device to whitelist. Later patches will make
> use of this whitelist to send conenct request when advertisement is
> received, and properly handle timeouts.
>
> Signed-off-by: Jakub Pawlowski <[email protected]>
> ---
> include/net/bluetooth/hci_core.h | 5 ++
> net/bluetooth/hci_conn.c | 170 +++++++++++++++++++++++++++++++++++++++
> net/bluetooth/hci_core.c | 32 ++++++++
> 3 files changed, 207 insertions(+)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index c8d2b5a..5d19794 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -826,6 +826,9 @@ void hci_chan_del(struct hci_chan *chan);
> void hci_chan_list_flush(struct hci_conn *conn);
> struct hci_chan *hci_chan_lookup_handle(struct hci_dev *hdev, __u16 handle);
>
> +struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst,
> + u8 dst_type, u8 sec_level,
> + u16 conn_timeout, u8 role);
> struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> u8 dst_type, u8 sec_level, u16 conn_timeout,
> u8 role);
> @@ -991,6 +994,8 @@ void hci_conn_params_clear_disabled(struct hci_dev *hdev);
> struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list,
> bdaddr_t *addr,
> u8 addr_type);
> +struct hci_conn_params *hci_pend_le_explicit_connect_lookup(
> + struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type);

this is kinda violating the coding style. I wonder if we can shorten the function name or just go over 80 characters this time.

> void hci_uuids_clear(struct hci_dev *hdev);
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 1ba8240..2a13214 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -64,6 +64,36 @@ static void hci_le_create_connection_cancel(struct hci_conn *conn)
> hci_send_cmd(conn->hdev, HCI_OP_LE_CREATE_CONN_CANCEL, 0, NULL);
> }
>
> +/* This function requires the caller holds hdev->lock */
> +static void hci_connect_le_scan_cleanup(struct hci_conn *conn)
> +{
> + struct hci_conn_params *params;
> +
> + params = hci_pend_le_explicit_connect_lookup(conn->hdev, &conn->dst,
> + conn->dst_type);
> + if (!params)
> + return;
> +
> + /* The connection attempt was doing scan for new RPA, and is
> + * in scan phase. If params are not associated with any other
> + * autoconnect action, remove them completely. If they are, just unmark
> + * them as awaiting for connection, by clearing explicit_connect field.
> + */
> + if (params->auto_connect == HCI_AUTO_CONN_EXPLICIT)
> + hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
> + else
> + params->explicit_connect = false;
> +}

What happens if we have two explicit connection requests here. I mean we can have two L2CAP socket trying to connect to two peripherals at the same time.

> +
> +/* This function requires the caller holds hdev->lock */
> +static void hci_connect_le_scan_remove(struct hci_conn *conn)
> +{
> + hci_connect_le_scan_cleanup(conn);
> +
> + hci_conn_hash_del(conn->hdev, conn);
> + hci_update_background_scan(conn->hdev);
> +}
> +
> static void hci_acl_create_connection(struct hci_conn *conn)
> {
> struct hci_dev *hdev = conn->hdev;
> @@ -858,6 +888,146 @@ done:
> return conn;
> }
>
> +static void hci_connect_le_scan_complete(struct hci_dev *hdev, u8 status,
> + u16 opcode)
> +{
> + struct hci_conn *conn;
> +
> + if (status == 0)
> + return;
> +

We prefer if (!status) these days.

> + BT_ERR("Failed to add device to auto conn whitelist: status 0x%2.2x",
> + status);
> +
> + hci_dev_lock(hdev);
> +
> + conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> + if (!conn)
> + goto done;
> +
> + hci_le_conn_failed(conn, status);
> +
> +done:

Remove the label here. Just move hci_le_conn_failed under if (conn) check.

> + hci_dev_unlock(hdev);
> +}
> +
> +static bool is_connected(struct hci_dev *hdev, bdaddr_t *addr, u8 type)
> +{
> + struct hci_conn *conn;
> +
> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, addr);
> + if (!conn)
> + return false;
> +
> + if (conn->dst_type != type)
> + return false;
> +
> + if (conn->state != BT_CONNECTED)
> + return false;
> +
> + return true;
> +}
> +
> +/* This function requires the caller holds hdev->lock */
> +int hci_explicit_conn_params_set(struct hci_request *req, bdaddr_t *addr,
> + u8 addr_type)
> +{
> + struct hci_dev *hdev = req->hdev;
> + struct hci_conn_params *params;
> +
> + if (is_connected(hdev, addr, addr_type))
> + return -EISCONN;
> +
> + params = hci_conn_params_add(hdev, addr, addr_type);
> + if (!params)
> + return -EIO;
> +
> + /* If we created new params, or exiting params were marked as disabled,
> + * mark them to be used just once to connect.
> + */
> + if (params->auto_connect == HCI_AUTO_CONN_DISABLED) {
> + params->auto_connect = HCI_AUTO_CONN_EXPLICIT;
> + list_del_init(&params->action);
> + list_add(&params->action, &hdev->pend_le_conns);
> + }
> +
> + params->explicit_connect = true;
> + __hci_update_background_scan(req);
> +
> + BT_DBG("addr %pMR (type %u) auto_connect %u", addr, addr_type,
> + params->auto_connect);
> +
> + return 0;
> +}
> +
> +/* This function requires the caller holds hdev->lock */
> +struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst,
> + u8 dst_type, u8 sec_level,
> + u16 conn_timeout, u8 role)
> +{
> + struct hci_conn *conn;
> + struct hci_request req;
> + int err;
> +
> + /* Let's make sure that le is enabled.*/
> + if (!hci_dev_test_flag(hdev, HCI_LE_ENABLED)) {
> + if (lmp_le_capable(hdev))
> + return ERR_PTR(-ECONNREFUSED);
> +
> + return ERR_PTR(-EOPNOTSUPP);
> + }
> +
> + /* Some devices send ATT messages as soon as the physical link is
> + * established. To be able to handle these ATT messages, the user-
> + * space first establishes the connection and then starts the pairing
> + * process.
> + *
> + * So if a hci_conn object already exists for the following connection
> + * attempt, we simply update pending_sec_level and auth_type fields
> + * and return the object found.
> + */
> + conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
> + if (conn) {
> + if (conn->pending_sec_level < sec_level)
> + conn->pending_sec_level = sec_level;
> + goto done;
> + }
> +
> + BT_DBG("requesting refresh of dst_addr.");
> +
> + /* We support only one connection attempt at a time. */
> + conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> + if (conn)
> + return ERR_PTR(-EBUSY);
> +
> + conn = hci_conn_add(hdev, LE_LINK, dst, role);
> + if (!conn)
> + return ERR_PTR(-ENOMEM);
> +
> + hci_req_init(&req, hdev);
> +
> + if (hci_explicit_conn_params_set(&req, dst, dst_type) < 0)
> + return ERR_PTR(-EBUSY);
> +
> + conn->state = BT_CONNECT;
> + set_bit(HCI_CONN_SCANNING, &conn->flags);
> +
> + err = hci_req_run(&req, hci_connect_le_scan_complete);
> + if (err && err != -ENODATA) {
> + hci_conn_del(conn);
> + return ERR_PTR(err);
> + }
> +
> + conn->dst_type = dst_type;
> + conn->sec_level = BT_SECURITY_LOW;
> + conn->pending_sec_level = sec_level;
> + conn->conn_timeout = conn_timeout;
> +
> +done:
> + hci_conn_hold(conn);
> + return conn;
> +}
> +
> struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
> u8 sec_level, u8 auth_type)
> {
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index bc43b64..e322aac 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2848,6 +2848,29 @@ struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list,
> }
>
> /* This function requires the caller holds hdev->lock */
> +struct hci_conn_params *hci_pend_le_explicit_connect_lookup(
> + struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
> +{
> + struct hci_conn_params *param;
> +
> + list_for_each_entry(param, &hdev->pend_le_conns, action) {
> + if (bacmp(&param->addr, addr) == 0 &&
> + param->addr_type == addr_type &&
> + param->explicit_connect)
> + return param;
> + }
> +
> + list_for_each_entry(param, &hdev->pend_le_reports, action) {
> + if (bacmp(&param->addr, addr) == 0 &&
> + param->addr_type == addr_type &&
> + param->explicit_connect)
> + return param;
> + }
> +
> + return NULL;
> +}
> +
> +/* This function requires the caller holds hdev->lock */
> struct hci_conn_params *hci_conn_params_add(struct hci_dev *hdev,
> bdaddr_t *addr, u8 addr_type)
> {
> @@ -2916,6 +2939,15 @@ void hci_conn_params_clear_disabled(struct hci_dev *hdev)
> list_for_each_entry_safe(params, tmp, &hdev->le_conn_params, list) {
> if (params->auto_connect != HCI_AUTO_CONN_DISABLED)
> continue;
> +
> + /* If trying to estabilish one time connection to disabled
> + * device, leave the params, but mark them as just once.
> + */
> + if (params->explicit_connect) {
> + params->auto_connect = HCI_AUTO_CONN_EXPLICIT;
> + continue;
> + }
> +
> list_del(&params->list);
> kfree(params);
> }

Regards

Marcel

2015-07-29 13:59:54

by Jakub Pawlowski

[permalink] [raw]

Subject: Re: [PATCH v5 1/6] Bluetooth: add HCI_LE_CONNECTING hci_dev flags

On Wed, Jul 29, 2015 at 3:56 PM, Marcel Holtmann <[email protected]> wrote:
>
> Hi Jakub,
>
> > This patch adds HCI_LE_CONNECTING flag to hci_dev flags, that will be set
> > when connect attempt is pending. It also uses this flag instead of conn
> > hash lookup where appropriate to check wether connect is in progress.
> >
> > Signed-off-by: Jakub Pawlowski <[email protected]>
> > ---
> > include/net/bluetooth/hci.h | 1 +
> > net/bluetooth/hci_conn.c | 3 +--
> > net/bluetooth/hci_event.c | 4 ++++
> > net/bluetooth/hci_request.c | 6 ++----
> > net/bluetooth/mgmt.c | 2 +-
> > 5 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 7ca6690..4772ff8 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -240,6 +240,7 @@ enum {
> > HCI_DUT_MODE,
> > HCI_FORCE_BREDR_SMP,
> > HCI_FORCE_STATIC_ADDR,
> > + HCI_LE_CONNECTING,
> >
> > __HCI_NUM_FLAGS,
> > };
>
> this needs to be added to hci_dev_clear_volatile_flags as well then. HCI_Reset would need to reset this value. And I would actually sort it after HCI_LE_SCAN flag or at least after HCI_LE_SCAN_INTERRUPTED.
>
> The last three are clearly put in an individual block since they are special and should be kept special. They are for debugging and testing purposes.
>
Ok, I fought I'm supposed to add it at end, will fix that

>
> Regards
>
> Marcel
>

2015-07-29 13:56:37

by Marcel Holtmann

[permalink] [raw]

Subject: Re: [PATCH v5 1/6] Bluetooth: add HCI_LE_CONNECTING hci_dev flags

Hi Jakub,

> This patch adds HCI_LE_CONNECTING flag to hci_dev flags, that will be set
> when connect attempt is pending. It also uses this flag instead of conn
> hash lookup where appropriate to check wether connect is in progress.
>
> Signed-off-by: Jakub Pawlowski <[email protected]>
> ---
> include/net/bluetooth/hci.h | 1 +
> net/bluetooth/hci_conn.c | 3 +--
> net/bluetooth/hci_event.c | 4 ++++
> net/bluetooth/hci_request.c | 6 ++----
> net/bluetooth/mgmt.c | 2 +-
> 5 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 7ca6690..4772ff8 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -240,6 +240,7 @@ enum {
> HCI_DUT_MODE,
> HCI_FORCE_BREDR_SMP,
> HCI_FORCE_STATIC_ADDR,
> + HCI_LE_CONNECTING,
>
> __HCI_NUM_FLAGS,
> };

this needs to be added to hci_dev_clear_volatile_flags as well then. HCI_Reset would need to reset this value. And I would actually sort it after HCI_LE_SCAN flag or at least after HCI_LE_SCAN_INTERRUPTED.

The last three are clearly put in an individual block since they are special and should be kept special. They are for debugging and testing purposes.

Regards

Marcel

2015-07-29 09:21:52

by Jakub Pawlowski

[permalink] [raw]

Subject: [PATCH v5 4/6] Bluetooth: advertisement handling in new connect procedure

Currently, when trying to connect to already paired device that just
rotated its RPA MAC address, old address would be used and connection
would fail. In order to fix that, kernel must scan and receive
advertisement with fresh RPA before connecting.

This path makes sure that after advertisement is received from device that
we try to connect to, it is properly handled in check_pending_le_conn and
trigger connect attempt.

It also modifies hci_le_connect to make sure that connect attempt will be
properly continued.

Signed-off-by: Jakub Pawlowski <[email protected]>
---
net/bluetooth/hci_conn.c | 37 ++++++++++++++++++++++++----------
net/bluetooth/hci_event.c | 51 +++++++++++++++++++++++++++--------------------
net/bluetooth/mgmt.c | 6 ++++++
3 files changed, 62 insertions(+), 32 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 2a13214..e433f6c 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -715,6 +715,7 @@ static void hci_req_add_le_create_conn(struct hci_request *req,
hci_req_add(req, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);

conn->state = BT_CONNECT;
+ clear_bit(HCI_CONN_SCANNING, &conn->flags);
}

static void hci_req_directed_advertising(struct hci_request *req,
@@ -758,7 +759,7 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
u8 role)
{
struct hci_conn_params *params;
- struct hci_conn *conn;
+ struct hci_conn *conn, *conn_unfinished;
struct smp_irk *irk;
struct hci_request req;
int err;
@@ -781,9 +782,17 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
* and return the object found.
*/
conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
+ conn_unfinished = NULL;
if (conn) {
- conn->pending_sec_level = sec_level;
- goto done;
+ if (conn->state == BT_CONNECT &&
+ test_bit(HCI_CONN_SCANNING, &conn->flags)) {
+ BT_DBG("will coninue unfinished conn %pMR", dst);
+ conn_unfinished = conn;
+ } else {
+ if (conn->pending_sec_level < sec_level)
+ conn->pending_sec_level = sec_level;
+ goto done;
+ }
}

/* Since the controller supports only one LE connection attempt at a
@@ -796,10 +805,6 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
* resolving key, the connection needs to be established
* to a resolvable random address.
*
- * This uses the cached random resolvable address from
- * a previous scan. When no cached address is available,
- * try connecting to the identity address instead.
- *
* Storing the resolvable random address is required here
* to handle connection failures. The address will later
* be resolved back into the original identity address
@@ -811,15 +816,23 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
dst_type = ADDR_LE_DEV_RANDOM;
}

- conn = hci_conn_add(hdev, LE_LINK, dst, role);
+ if (conn_unfinished) {
+ conn = conn_unfinished;
+ bacpy(&conn->dst, dst);
+ } else {
+ conn = hci_conn_add(hdev, LE_LINK, dst, role);
+ }
+
if (!conn)
return ERR_PTR(-ENOMEM);

conn->dst_type = dst_type;
conn->sec_level = BT_SECURITY_LOW;
- conn->pending_sec_level = sec_level;
conn->conn_timeout = conn_timeout;

+ if (!conn_unfinished)
+ conn->pending_sec_level = sec_level;
+
hci_req_init(&req, hdev);

/* Disable advertising if we're active. For master role
@@ -884,7 +897,11 @@ create_conn:
}

done:
- hci_conn_hold(conn);
+ if (conn_unfinished)
+ hci_connect_le_scan_cleanup(conn);
+ else
+ hci_conn_hold(conn);
+
return conn;
}

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 18fccf8..eada39b 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4737,42 +4737,49 @@ static struct hci_conn *check_pending_le_conn(struct hci_dev *hdev,
/* If we're not connectable only connect devices that we have in
* our pend_le_conns list.
*/
- params = hci_pend_le_action_lookup(&hdev->pend_le_conns,
- addr, addr_type);
+ params = hci_pend_le_explicit_connect_lookup(hdev, addr, addr_type);
+
if (!params)
return NULL;

- switch (params->auto_connect) {
- case HCI_AUTO_CONN_DIRECT:
- /* Only devices advertising with ADV_DIRECT_IND are
- * triggering a connection attempt. This is allowing
- * incoming connections from slave devices.
- */
- if (adv_type != LE_ADV_DIRECT_IND)
+ if (!params->explicit_connect) {
+ switch (params->auto_connect) {
+ case HCI_AUTO_CONN_DIRECT:
+ /* Only devices advertising with ADV_DIRECT_IND are
+ * triggering a connection attempt. This is allowing
+ * incoming connections from slave devices.
+ */
+ if (adv_type != LE_ADV_DIRECT_IND)
+ return NULL;
+ break;
+ case HCI_AUTO_CONN_ALWAYS:
+ /* Devices advertising with ADV_IND or ADV_DIRECT_IND
+ * are triggering a connection attempt. This means
+ * that incoming connectioms from slave device are
+ * accepted and also outgoing connections to slave
+ * devices are established when found.
+ */
+ break;
+ default:
return NULL;
- break;
- case HCI_AUTO_CONN_ALWAYS:
- /* Devices advertising with ADV_IND or ADV_DIRECT_IND
- * are triggering a connection attempt. This means
- * that incoming connectioms from slave device are
- * accepted and also outgoing connections to slave
- * devices are established when found.
- */
- break;
- default:
- return NULL;
+ }
}

conn = hci_connect_le(hdev, addr, addr_type, BT_SECURITY_LOW,
HCI_LE_AUTOCONN_TIMEOUT, HCI_ROLE_MASTER);
if (!IS_ERR(conn)) {
- /* Store the pointer since we don't really have any
+ /* If HCI_AUTO_CONN_EXPLICIT is set, conn is already owned
+ * by higher layer that tried to connect, if no then
+ * store the pointer since we don't really have any
* other owner of the object besides the params that
* triggered it. This way we can abort the connection if
* the parameters get removed and keep the reference
* count consistent once the connection is established.
*/
- params->conn = hci_conn_get(conn);
+
+ if (!params->explicit_connect)
+ params->conn = hci_conn_get(conn);
+
return conn;
}

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index c35d671..fac6582 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -6107,6 +6107,12 @@ static int hci_conn_params_set(struct hci_request *req, bdaddr_t *addr,
switch (auto_connect) {
case HCI_AUTO_CONN_DISABLED:
case HCI_AUTO_CONN_LINK_LOSS:
+ /* If auto connect is being disabled when we're trying to
+ * connect to device, keep connecting.
+ */
+ if (params->explicit_connect)
+ list_add(&params->action, &hdev->pend_le_conns);
+
__hci_update_background_scan(req);
break;
case HCI_AUTO_CONN_REPORT:
--
2.1.4

2015-07-29 09:21:51

by Jakub Pawlowski

[permalink] [raw]

Subject: [PATCH v5 3/6] Bluetooth: add hci_connect_le_scan

Currently, when trying to connect to already paired device that just
rotated its RPA MAC address, old address would be used and connection
would fail. In order to fix that, kernel must scan and receive
advertisement with fresh RPA before connecting.

This patch adds hci_connect_le_scan with dependencies, new method that
will be used to connect to remote LE devices. Instead of just sending
connect request, it adds a device to whitelist. Later patches will make
use of this whitelist to send conenct request when advertisement is
received, and properly handle timeouts.

Signed-off-by: Jakub Pawlowski <[email protected]>
---
include/net/bluetooth/hci_core.h | 5 ++
net/bluetooth/hci_conn.c | 170 +++++++++++++++++++++++++++++++++++++++
net/bluetooth/hci_core.c | 32 ++++++++
3 files changed, 207 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index c8d2b5a..5d19794 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -826,6 +826,9 @@ void hci_chan_del(struct hci_chan *chan);
void hci_chan_list_flush(struct hci_conn *conn);
struct hci_chan *hci_chan_lookup_handle(struct hci_dev *hdev, __u16 handle);

+struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst,
+ u8 dst_type, u8 sec_level,
+ u16 conn_timeout, u8 role);
struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
u8 dst_type, u8 sec_level, u16 conn_timeout,
u8 role);
@@ -991,6 +994,8 @@ void hci_conn_params_clear_disabled(struct hci_dev *hdev);
struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list,
bdaddr_t *addr,
u8 addr_type);
+struct hci_conn_params *hci_pend_le_explicit_connect_lookup(
+ struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type);

void hci_uuids_clear(struct hci_dev *hdev);

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 1ba8240..2a13214 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -64,6 +64,36 @@ static void hci_le_create_connection_cancel(struct hci_conn *conn)
hci_send_cmd(conn->hdev, HCI_OP_LE_CREATE_CONN_CANCEL, 0, NULL);
}

+/* This function requires the caller holds hdev->lock */
+static void hci_connect_le_scan_cleanup(struct hci_conn *conn)
+{
+ struct hci_conn_params *params;
+
+ params = hci_pend_le_explicit_connect_lookup(conn->hdev, &conn->dst,
+ conn->dst_type);
+ if (!params)
+ return;
+
+ /* The connection attempt was doing scan for new RPA, and is
+ * in scan phase. If params are not associated with any other
+ * autoconnect action, remove them completely. If they are, just unmark
+ * them as awaiting for connection, by clearing explicit_connect field.
+ */
+ if (params->auto_connect == HCI_AUTO_CONN_EXPLICIT)
+ hci_conn_params_del(conn->hdev, &conn->dst, conn->dst_type);
+ else
+ params->explicit_connect = false;
+}
+
+/* This function requires the caller holds hdev->lock */
+static void hci_connect_le_scan_remove(struct hci_conn *conn)
+{
+ hci_connect_le_scan_cleanup(conn);
+
+ hci_conn_hash_del(conn->hdev, conn);
+ hci_update_background_scan(conn->hdev);
+}
+
static void hci_acl_create_connection(struct hci_conn *conn)
{
struct hci_dev *hdev = conn->hdev;
@@ -858,6 +888,146 @@ done:
return conn;
}

+static void hci_connect_le_scan_complete(struct hci_dev *hdev, u8 status,
+ u16 opcode)
+{
+ struct hci_conn *conn;
+
+ if (status == 0)
+ return;
+
+ BT_ERR("Failed to add device to auto conn whitelist: status 0x%2.2x",
+ status);
+
+ hci_dev_lock(hdev);
+
+ conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
+ if (!conn)
+ goto done;
+
+ hci_le_conn_failed(conn, status);
+
+done:
+ hci_dev_unlock(hdev);
+}
+
+static bool is_connected(struct hci_dev *hdev, bdaddr_t *addr, u8 type)
+{
+ struct hci_conn *conn;
+
+ conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, addr);
+ if (!conn)
+ return false;
+
+ if (conn->dst_type != type)
+ return false;
+
+ if (conn->state != BT_CONNECTED)
+ return false;
+
+ return true;
+}
+
+/* This function requires the caller holds hdev->lock */
+int hci_explicit_conn_params_set(struct hci_request *req, bdaddr_t *addr,
+ u8 addr_type)
+{
+ struct hci_dev *hdev = req->hdev;
+ struct hci_conn_params *params;
+
+ if (is_connected(hdev, addr, addr_type))
+ return -EISCONN;
+
+ params = hci_conn_params_add(hdev, addr, addr_type);
+ if (!params)
+ return -EIO;
+
+ /* If we created new params, or exiting params were marked as disabled,
+ * mark them to be used just once to connect.
+ */
+ if (params->auto_connect == HCI_AUTO_CONN_DISABLED) {
+ params->auto_connect = HCI_AUTO_CONN_EXPLICIT;
+ list_del_init(&params->action);
+ list_add(&params->action, &hdev->pend_le_conns);
+ }
+
+ params->explicit_connect = true;
+ __hci_update_background_scan(req);
+
+ BT_DBG("addr %pMR (type %u) auto_connect %u", addr, addr_type,
+ params->auto_connect);
+
+ return 0;
+}
+
+/* This function requires the caller holds hdev->lock */
+struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst,
+ u8 dst_type, u8 sec_level,
+ u16 conn_timeout, u8 role)
+{
+ struct hci_conn *conn;
+ struct hci_request req;
+ int err;
+
+ /* Let's make sure that le is enabled.*/
+ if (!hci_dev_test_flag(hdev, HCI_LE_ENABLED)) {
+ if (lmp_le_capable(hdev))
+ return ERR_PTR(-ECONNREFUSED);
+
+ return ERR_PTR(-EOPNOTSUPP);
+ }
+
+ /* Some devices send ATT messages as soon as the physical link is
+ * established. To be able to handle these ATT messages, the user-
+ * space first establishes the connection and then starts the pairing
+ * process.
+ *
+ * So if a hci_conn object already exists for the following connection
+ * attempt, we simply update pending_sec_level and auth_type fields
+ * and return the object found.
+ */
+ conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
+ if (conn) {
+ if (conn->pending_sec_level < sec_level)
+ conn->pending_sec_level = sec_level;
+ goto done;
+ }
+
+ BT_DBG("requesting refresh of dst_addr.");
+
+ /* We support only one connection attempt at a time. */
+ conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
+ if (conn)
+ return ERR_PTR(-EBUSY);
+
+ conn = hci_conn_add(hdev, LE_LINK, dst, role);
+ if (!conn)
+ return ERR_PTR(-ENOMEM);
+
+ hci_req_init(&req, hdev);
+
+ if (hci_explicit_conn_params_set(&req, dst, dst_type) < 0)
+ return ERR_PTR(-EBUSY);
+
+ conn->state = BT_CONNECT;
+ set_bit(HCI_CONN_SCANNING, &conn->flags);
+
+ err = hci_req_run(&req, hci_connect_le_scan_complete);
+ if (err && err != -ENODATA) {
+ hci_conn_del(conn);
+ return ERR_PTR(err);
+ }
+
+ conn->dst_type = dst_type;
+ conn->sec_level = BT_SECURITY_LOW;
+ conn->pending_sec_level = sec_level;
+ conn->conn_timeout = conn_timeout;
+
+done:
+ hci_conn_hold(conn);
+ return conn;
+}
+
struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
u8 sec_level, u8 auth_type)
{
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index bc43b64..e322aac 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2848,6 +2848,29 @@ struct hci_conn_params *hci_pend_le_action_lookup(struct list_head *list,
}

/* This function requires the caller holds hdev->lock */
+struct hci_conn_params *hci_pend_le_explicit_connect_lookup(
+ struct hci_dev *hdev, bdaddr_t *addr, u8 addr_type)
+{
+ struct hci_conn_params *param;
+
+ list_for_each_entry(param, &hdev->pend_le_conns, action) {
+ if (bacmp(&param->addr, addr) == 0 &&
+ param->addr_type == addr_type &&
+ param->explicit_connect)
+ return param;
+ }
+
+ list_for_each_entry(param, &hdev->pend_le_reports, action) {
+ if (bacmp(&param->addr, addr) == 0 &&
+ param->addr_type == addr_type &&
+ param->explicit_connect)
+ return param;
+ }
+
+ return NULL;
+}
+
+/* This function requires the caller holds hdev->lock */
struct hci_conn_params *hci_conn_params_add(struct hci_dev *hdev,
bdaddr_t *addr, u8 addr_type)
{
@@ -2916,6 +2939,15 @@ void hci_conn_params_clear_disabled(struct hci_dev *hdev)
list_for_each_entry_safe(params, tmp, &hdev->le_conn_params, list) {
if (params->auto_connect != HCI_AUTO_CONN_DISABLED)
continue;
+
+ /* If trying to estabilish one time connection to disabled
+ * device, leave the params, but mark them as just once.
+ */
+ if (params->explicit_connect) {
+ params->auto_connect = HCI_AUTO_CONN_EXPLICIT;
+ continue;
+ }
+
list_del(&params->list);
kfree(params);
}
--
2.1.4