2014-10-06 23:12:29

by Alfonso Acosta

[permalink] [raw]
Subject: [PATCH v4] Bluetooth: Include ADV_IND report in Device Connected event

There are scenarios when autoconnecting to a device after the
reception of an ADV_IND report (action 0x02), in which userland
might want to examine the report's contents.

For instance, the Service Data might have changed and it would be
useful to know ahead of time before starting any GATT procedures.
Also, the ADV_IND may contain Manufacturer Specific data which would
be lost if not propagated to userland. In fact, this patch results
from the need to rebond with a device lacking persistent storage which
notifies about losing its LTK in ADV_IND reports.

This patch appends the ADV_IND report which triggered the
autoconnection to the EIR Data in the Device Connected event.

Signed-off-by: Alfonso Acosta <[email protected]>
---
include/net/bluetooth/hci_core.h | 2 ++
net/bluetooth/hci_event.c | 32 ++++++++++++++++++++++----------
net/bluetooth/mgmt.c | 24 ++++++++++++++++++------
3 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index f1407fe..07ddeed62 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -398,6 +398,8 @@ struct hci_conn {
__u16 le_conn_interval;
__u16 le_conn_latency;
__u16 le_supv_timeout;
+ __u8 le_adv_data[HCI_MAX_AD_LENGTH];
+ __u8 le_adv_data_len;
__s8 rssi;
__s8 tx_power;
__s8 max_tx_power;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 6ee7de2..1b51c29 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4263,25 +4263,26 @@ static void hci_le_conn_update_complete_evt(struct hci_dev *hdev,
}

/* This function requires the caller holds hdev->lock */
-static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
- u8 addr_type, u8 adv_type)
+static struct hci_conn *check_pending_le_conn(struct hci_dev *hdev,
+ bdaddr_t *addr,
+ u8 addr_type, u8 adv_type)
{
struct hci_conn *conn;
struct hci_conn_params *params;

/* If the event is not connectable don't proceed further */
if (adv_type != LE_ADV_IND && adv_type != LE_ADV_DIRECT_IND)
- return;
+ return NULL;

/* Ignore if the device is blocked */
if (hci_bdaddr_list_lookup(&hdev->blacklist, addr, addr_type))
- return;
+ return NULL;

/* Most controller will fail if we try to create new connections
* while we have an existing one in slave role.
*/
if (hdev->conn_hash.le_num_slave > 0)
- return;
+ return NULL;

/* If we're not connectable only connect devices that we have in
* our pend_le_conns list.
@@ -4289,7 +4290,7 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
params = hci_pend_le_action_lookup(&hdev->pend_le_conns,
addr, addr_type);
if (!params)
- return;
+ return NULL;

switch (params->auto_connect) {
case HCI_AUTO_CONN_DIRECT:
@@ -4298,7 +4299,7 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
* incoming connections from slave devices.
*/
if (adv_type != LE_ADV_DIRECT_IND)
- return;
+ return NULL;
break;
case HCI_AUTO_CONN_ALWAYS:
/* Devices advertising with ADV_IND or ADV_DIRECT_IND
@@ -4309,7 +4310,7 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
*/
break;
default:
- return;
+ return NULL;
}

conn = hci_connect_le(hdev, addr, addr_type, BT_SECURITY_LOW,
@@ -4322,7 +4323,7 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
* count consistent once the connection is established.
*/
params->conn = hci_conn_get(conn);
- return;
+ return conn;
}

switch (PTR_ERR(conn)) {
@@ -4335,7 +4336,10 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
break;
default:
BT_DBG("Failed to connect: err %ld", PTR_ERR(conn));
+ return NULL;
}
+
+ return conn;
}

static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
@@ -4343,6 +4347,7 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
{
struct discovery_state *d = &hdev->discovery;
struct smp_irk *irk;
+ struct hci_conn *conn;
bool match;
u32 flags;

@@ -4354,7 +4359,14 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
}

/* Check if we have been requested to connect to this device */
- check_pending_le_conn(hdev, bdaddr, bdaddr_type, type);
+ conn = check_pending_le_conn(hdev, bdaddr, bdaddr_type, type);
+ if (conn && type == LE_ADV_IND) {
+ /* Store report for later inclusion by
+ * mgmt_device_connected
+ */
+ memcpy(conn->le_adv_data, data, len);
+ conn->le_adv_data_len = len;
+ }

/* Passive scanning shouldn't trigger any device found events,
* except for devices marked as CONN_REPORT for which we do send
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index fc275dc..10caab5 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -6183,13 +6183,25 @@ void mgmt_device_connected(struct hci_dev *hdev, struct hci_conn *conn,

ev->flags = __cpu_to_le32(flags);

- if (name_len > 0)
- eir_len = eir_append_data(ev->eir, 0, EIR_NAME_COMPLETE,
- name, name_len);
+ /* We must ensure that the EIR Data fields are ordered and
+ * unique. Keep it simple for now and avoid the problem by not
+ * adding any BR/EDR data to the LE adv.
+ */
+ if (conn->le_adv_data_len > 0) {
+ memcpy(&ev->eir[eir_len],
+ conn->le_adv_data, conn->le_adv_data_len);
+ eir_len = conn->le_adv_data_len;
+ } else {
+ if (name_len > 0)
+ eir_len = eir_append_data(ev->eir, 0, EIR_NAME_COMPLETE,
+ name, name_len);

- if (conn->dev_class && memcmp(conn->dev_class, "\0\0\0", 3) != 0)
- eir_len = eir_append_data(ev->eir, eir_len,
- EIR_CLASS_OF_DEV, conn->dev_class, 3);
+ if (conn->dev_class &&
+ memcmp(conn->dev_class, "\0\0\0", 3) != 0)
+ eir_len = eir_append_data(ev->eir, eir_len,
+ EIR_CLASS_OF_DEV,
+ conn->dev_class, 3);
+ }

ev->eir_len = cpu_to_le16(eir_len);

--
1.9.1



2014-10-07 09:30:33

by Alfonso Acosta

[permalink] [raw]
Subject: Re: [PATCH v4] Bluetooth: Include ADV_IND report in Device Connected event

Hi Johan,


> On Mon, Oct 06, 2014, Alfonso Acosta wrote:
>> @@ -4322,7 +4323,7 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
>> * count consistent once the connection is established.
>> */
>> params->conn = hci_conn_get(conn);
>> - return;
>> + return conn;
>> }
>>
>> switch (PTR_ERR(conn)) {
>> @@ -4335,7 +4336,10 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
>> break;
>> default:
>> BT_DBG("Failed to connect: err %ld", PTR_ERR(conn));
>> + return NULL;
>> }
>> +
>> + return conn;
>> }
>
> I just realized that in this last part of check_pending_le_conn()
> IS_ERR(conn) will true (since the first part that I quoted handles the
> !IS_ERR(conn) condition) so you should really be explicitly returning
> NULL here instead of the conn pointer. This is particularly important
> since you're only checking for non-NULL in the calling code instead of
> doing IS_ERR() there.
>
>> - if (conn->dev_class && memcmp(conn->dev_class, "\0\0\0", 3) != 0)
>> - eir_len = eir_append_data(ev->eir, eir_len,
>> - EIR_CLASS_OF_DEV, conn->dev_class, 3);
>> + if (conn->dev_class &&
>> + memcmp(conn->dev_class, "\0\0\0", 3) != 0)
>> + eir_len = eir_append_data(ev->eir, eir_len,
>> + EIR_CLASS_OF_DEV,
>> + conn->dev_class, 3);
>> + }
>
> I know this is not your fault but a redundancy in the existing code, but
> the check for if (conn->dev_class) is pointless since the variable is
> defined as an array, i.e. it will always be non-NULL. To make this fix
> clear it's probably better to have it as a separate patch either before
> or after your current two patches.
>
> To make it easier to get the right versions of these applied could you
> please send the entire set again instead of just this one patch. Thanks.

The v5 bundle takes care of both remarks.


--
Alfonso Acosta

Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com

2014-10-07 08:01:54

by Johan Hedberg

[permalink] [raw]
Subject: Re: [PATCH v4] Bluetooth: Include ADV_IND report in Device Connected event

Hi Alfonso,

On Mon, Oct 06, 2014, Alfonso Acosta wrote:
> @@ -4322,7 +4323,7 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
> * count consistent once the connection is established.
> */
> params->conn = hci_conn_get(conn);
> - return;
> + return conn;
> }
>
> switch (PTR_ERR(conn)) {
> @@ -4335,7 +4336,10 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
> break;
> default:
> BT_DBG("Failed to connect: err %ld", PTR_ERR(conn));
> + return NULL;
> }
> +
> + return conn;
> }

I just realized that in this last part of check_pending_le_conn()
IS_ERR(conn) will true (since the first part that I quoted handles the
!IS_ERR(conn) condition) so you should really be explicitly returning
NULL here instead of the conn pointer. This is particularly important
since you're only checking for non-NULL in the calling code instead of
doing IS_ERR() there.

> - if (conn->dev_class && memcmp(conn->dev_class, "\0\0\0", 3) != 0)
> - eir_len = eir_append_data(ev->eir, eir_len,
> - EIR_CLASS_OF_DEV, conn->dev_class, 3);
> + if (conn->dev_class &&
> + memcmp(conn->dev_class, "\0\0\0", 3) != 0)
> + eir_len = eir_append_data(ev->eir, eir_len,
> + EIR_CLASS_OF_DEV,
> + conn->dev_class, 3);
> + }

I know this is not your fault but a redundancy in the existing code, but
the check for if (conn->dev_class) is pointless since the variable is
defined as an array, i.e. it will always be non-NULL. To make this fix
clear it's probably better to have it as a separate patch either before
or after your current two patches.

To make it easier to get the right versions of these applied could you
please send the entire set again instead of just this one patch. Thanks.

Johan

2014-10-06 23:14:02

by Alfonso Acosta

[permalink] [raw]
Subject: Re: [PATCH v3] Bluetooth: Include ADV_IND report in Device Connected event

Hi Johan,


> On Fri, Oct 03, 2014, Alfonso Acosta wrote:
>> void mgmt_device_connected(struct hci_dev *hdev, struct hci_conn *conn,
>> - u32 flags, u8 *name, u8 name_len);
>> + u32 flags, u8 *name, u8 name_len,
>> + u8 *adv, u8 adv_len);
>
> Now that you're already passing the full hci_conn to this function I
> guess it's a bit pointless to add new parameters to it which would
> anyway come from that same hci_conn object.

True, I didn't think about it when merging the second patch.


>> + /* We must ensure that the EIR Data fields are ordered and
>> + * unique. Keep it simple for now and avoid the problem by not
>> + * adding any BR/EDR data to the LE adv.
>> + */
>> + if (adv) {
>
> And once you do the earlier fix I suppose this check becomes:
>
> if (conn->le_adv_data_len > 0) {


Both remarks have been taken care of in version 4.

Thanks,

Fons



--
Alfonso Acosta

Embedded Systems Engineer at Spotify
Birger Jarlsgatan 61, Stockholm, Sweden
http://www.spotify.com