Hi Gustavo,
According to what we discussed, the main changes in this v2 is the definition
of lmp_bredr_capable macro instead of lmp_no_bredr_capable.
Thanks,
Andre.
Andre Guedes (16):
Bluetooth: Periodic Inquiry and mgmt discovering event
Bluetooth: Add failed/complete functions to discovery commands
Bluetooth: Remove pending discovery commands
Bluetooth: Check pending command in start_discovery()
Bluetooth: Check pending commands in stop_discovery()
Bluetooth: Create do_inquiry()
Bluetooth: Create cancel_inquiry()
Bluetooth: Fix stop_discovery()
Bluetooth: Prepare for full support discovery procedures
Bluetooth: Check 'dev_class' in mgmt_device_found()
Bluetooth: Add 'eir_len' param to mgmt_device_found()
Bluetooth: Report LE devices
Bluetooth: Add 'le_scan_timer' to struct hci_dev
Bluetooth: Add LE Scan helper functions
Bluetooth: Support LE-Only discovery procedure
Bluetooth: Support BR/EDR/LE discovery procedure
include/net/bluetooth/hci.h | 12 ++
include/net/bluetooth/hci_core.h | 13 ++-
net/bluetooth/hci_core.c | 13 ++
net/bluetooth/hci_event.c | 134 +++++++++++++------
net/bluetooth/mgmt.c | 282 ++++++++++++++++++++++++++++++++++++--
5 files changed, 402 insertions(+), 52 deletions(-)
--
1.7.4.1
This patch adds support for BR/EDR/LE discovery procedure through
management interface.
Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci_core.h | 2 ++
net/bluetooth/hci_event.c | 8 +++++++-
net/bluetooth/mgmt.c | 38 +++++++++++++++++++++++++++++++++++++-
3 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index f23102a..1bc1c3a 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -866,6 +866,8 @@ int mgmt_stop_discovery_complete(u16 index);
int mgmt_stop_discovery_failed(u16 index, u8 status);
int mgmt_has_pending_stop_discov(u16 index);
int mgmt_cancel_discovery(u16 index);
+int mgmt_is_interleaved_discovery(u16 index);
+int mgmt_do_interleaved_discovery(u16 index);
/* HCI info for socket */
#define hci_pi(sk) ((struct hci_pinfo *) sk)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 08774c2..55872ff 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -913,7 +913,8 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
goto unlock;
}
- mgmt_discovering(hdev->id, 1);
+ if (!mgmt_is_interleaved_discovery(hdev->id))
+ mgmt_discovering(hdev->id, 1);
} else if (cp->enable == 0x00) {
if (status) {
if (mgmt_has_pending_stop_discov(hdev->id))
@@ -1393,6 +1394,11 @@ static inline void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff
hci_req_complete(hdev, HCI_OP_INQUIRY, status);
+ if (mgmt_is_interleaved_discovery(hdev->id)) {
+ mgmt_do_interleaved_discovery(hdev->id);
+ return;
+ }
+
mgmt_discovering(hdev->id, 0);
hci_dev_lock(hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index db3b177..7c0f4f6 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -40,6 +40,8 @@ enum bt_device_type {
};
#define BREDR_ONLY_INQ_LENGTH 0x08 /* TGAP(100) */
+#define BREDR_LE_INQ_LENGTH 0x04 /* TGAP(100)/2 */
+#define BREDR_LE_SCAN_TIMEOUT 5120 /* TGAP(100)/2 */
#define LE_ONLY_SCAN_TIMEOUT 10240 /* TGAP(gen_disc_scan_min) */
struct pending_cmd {
@@ -1731,7 +1733,7 @@ static int start_discovery(struct sock *sk, u16 index)
err = do_le_scan(hdev, LE_ONLY_SCAN_TIMEOUT);
break;
case BREDR_LE:
- err = -ENOSYS;
+ err = do_inquiry(hdev, BREDR_LE_INQ_LENGTH);
break;
default:
err = -EINVAL;
@@ -2512,3 +2514,37 @@ int mgmt_has_pending_stop_discov(u16 index)
return 0;
}
+
+int mgmt_is_interleaved_discovery(u16 index)
+{
+ struct hci_dev *hdev;
+ int res = 0;
+
+ hdev = hci_dev_get(index);
+
+ if (get_device_type(hdev) == BREDR_LE &&
+ mgmt_pending_find(MGMT_OP_START_DISCOVERY, hdev->id))
+ res = 1;
+
+ hci_dev_put(hdev);
+
+ return res;
+}
+
+int mgmt_do_interleaved_discovery(u16 index)
+{
+ struct hci_dev *hdev;
+ int err;
+
+ hdev = hci_dev_get(index);
+
+ err = do_le_scan(hdev, BREDR_LE_SCAN_TIMEOUT);
+ if (err < 0) {
+ mgmt_discovering(hdev->id, 0);
+ mgmt_start_discovery_failed(hdev->id, err);
+ }
+
+ hci_dev_put(hdev);
+
+ return err;
+}
--
1.7.4.1
This patch adds support for LE-Only discovery procedure through
management interface.
A new flag (HCI_LE_SCAN) was created to inform if the controller is
performing LE scan. The HCI_LE_SCAN flag is set/cleared when the
controller starts/stops scanning.
Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci.h | 2 ++
net/bluetooth/hci_event.c | 39 ++++++++++++++++++++++++++++++++++++---
net/bluetooth/mgmt.c | 5 +++++
3 files changed, 43 insertions(+), 3 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index fb40388..c4fdeeb 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -86,6 +86,8 @@ enum {
HCI_DEBUG_KEYS,
HCI_RESET,
+
+ HCI_LE_SCAN,
};
/* HCI ioctl defines */
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index a4f0929..08774c2 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -890,9 +890,6 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
BT_DBG("%s status 0x%x", hdev->name, status);
- if (status)
- return;
-
cp = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_SCAN_ENABLE);
if (!cp)
return;
@@ -900,12 +897,48 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
hci_dev_lock(hdev);
if (cp->enable == 0x01) {
+ if (status) {
+ mgmt_start_discovery_failed(hdev->id,
+ bt_to_errno(status));
+ goto unlock;
+ }
+
+ set_bit(HCI_LE_SCAN, &hdev->flags);
+
del_timer(&hdev->adv_timer);
hci_adv_entries_clear(hdev);
+
+ if (mgmt_has_pending_stop_discov(hdev->id)) {
+ mgmt_cancel_discovery(hdev->id);
+ goto unlock;
+ }
+
+ mgmt_discovering(hdev->id, 1);
} else if (cp->enable == 0x00) {
+ if (status) {
+ if (mgmt_has_pending_stop_discov(hdev->id))
+ mgmt_stop_discovery_failed(hdev->id,
+ bt_to_errno(status));
+ else
+ mgmt_start_discovery_failed(hdev->id,
+ bt_to_errno(status));
+
+ goto unlock;
+ }
+
+ clear_bit(HCI_LE_SCAN, &hdev->flags);
+
mod_timer(&hdev->adv_timer, jiffies + ADV_CLEAR_TIMEOUT);
+
+ mgmt_discovering(hdev->id, 0);
+
+ if (mgmt_has_pending_stop_discov(hdev->id))
+ mgmt_stop_discovery_complete(hdev->id);
+ else
+ mgmt_start_discovery_complete(hdev->id);
}
+unlock:
hci_dev_unlock(hdev);
}
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index c805377..db3b177 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -40,6 +40,7 @@ enum bt_device_type {
};
#define BREDR_ONLY_INQ_LENGTH 0x08 /* TGAP(100) */
+#define LE_ONLY_SCAN_TIMEOUT 10240 /* TGAP(gen_disc_scan_min) */
struct pending_cmd {
struct list_head list;
@@ -1727,6 +1728,8 @@ static int start_discovery(struct sock *sk, u16 index)
err = do_inquiry(hdev, BREDR_ONLY_INQ_LENGTH);
break;
case LE_ONLY:
+ err = do_le_scan(hdev, LE_ONLY_SCAN_TIMEOUT);
+ break;
case BREDR_LE:
err = -ENOSYS;
break;
@@ -1765,6 +1768,8 @@ int mgmt_cancel_discovery(u16 index)
if (test_bit(HCI_INQUIRY, &hdev->flags))
res = cancel_inquiry(hdev);
+ else if (test_bit(HCI_LE_SCAN, &hdev->flags))
+ res = cancel_le_scan(hdev);
hci_dev_put(hdev);
--
1.7.4.1
This patch adds some helper functions to perform LE scan according
to the general discovery procedure recommendations.
Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci.h | 9 +++++++
net/bluetooth/mgmt.c | 51 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 60 insertions(+), 0 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 653daec..fb40388 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -729,6 +729,15 @@ struct hci_rp_le_read_buffer_size {
__u8 le_max_pkt;
} __packed;
+#define HCI_OP_LE_SET_SCAN_PARAM 0x200b
+struct hci_cp_le_set_scan_param {
+ __u8 type;
+ __le16 interval;
+ __le16 window;
+ __u8 own_address_type;
+ __u8 filter_policy;
+} __packed;
+
#define HCI_OP_LE_SET_SCAN_ENABLE 0x200c
struct hci_cp_le_set_scan_enable {
__u8 enable;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index c0d3321..c805377 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1637,6 +1637,50 @@ static int do_inquiry(struct hci_dev *hdev, __u8 inq_length)
return hci_send_cmd(hdev, HCI_OP_INQUIRY, sizeof(cp), &cp);
}
+static int set_le_scan_param(struct hci_dev *hdev)
+{
+ struct hci_cp_le_set_scan_param cp;
+
+ /*
+ * These values are set according to LE General Discovery Procedure
+ * specification.
+ */
+ cp.type = 0x01; /* Active scanning */
+ cp.interval = cpu_to_le16(0x0012); /* TGAP(gen_disc_scan_int) */
+ cp.window = cpu_to_le16(0x0012); /* TGAP(gen_disc_scan_wind) */
+ cp.own_address_type = 0x00; /* Public */
+ cp.filter_policy = 0x00; /* Accept all advertisement packets */
+
+ return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_PARAM, sizeof(cp), &cp);
+}
+
+static int enable_le_scan(struct hci_dev *hdev, __u8 enable)
+{
+ struct hci_cp_le_set_scan_enable cp;
+
+ memset(&cp, 0, sizeof(cp));
+ cp.enable = enable;
+
+ return hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
+}
+
+static int do_le_scan(struct hci_dev *hdev, int timeout)
+{
+ int err;
+
+ err = set_le_scan_param(hdev);
+ if (err < 0)
+ return err;
+
+ err = enable_le_scan(hdev, 1);
+ if (err < 0)
+ return err;
+
+ mod_timer(&hdev->le_scan_timer, jiffies + msecs_to_jiffies(timeout));
+
+ return 0;
+}
+
static int get_device_type(struct hci_dev *hdev)
{
if (lmp_bredr_capable(hdev) && lmp_host_le_capable(hdev))
@@ -1705,6 +1749,13 @@ static int cancel_inquiry(struct hci_dev *hdev)
return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
}
+static int cancel_le_scan(struct hci_dev *hdev)
+{
+ del_timer(&hdev->le_scan_timer);
+
+ return enable_le_scan(hdev, 0);
+}
+
int mgmt_cancel_discovery(u16 index)
{
struct hci_dev *hdev;
--
1.7.4.1
This patch adds a timer to disable LE Scan after some amount of
time. The timer will be controlled by the management interface and
it will be used to carry out discovery procedure in LE capable
devices.
Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci_core.h | 2 ++
net/bluetooth/hci_core.c | 13 +++++++++++++
2 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 1ca4e60..f23102a 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -209,6 +209,8 @@ struct hci_dev {
struct list_head adv_entries;
struct timer_list adv_timer;
+ struct timer_list le_scan_timer;
+
struct hci_dev_stats stat;
struct sk_buff_head driver_init;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 908fcd3..c0c46bf 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1440,6 +1440,15 @@ int hci_add_adv_entry(struct hci_dev *hdev,
return 0;
}
+static void hci_disable_le_scan(unsigned long arg)
+{
+ struct hci_cp_le_set_scan_enable cp;
+ struct hci_dev *hdev = (void *) arg;
+
+ memset(&cp, 0, sizeof(cp));
+ hci_send_cmd(hdev, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
+}
+
/* Register HCI device */
int hci_register_dev(struct hci_dev *hdev)
{
@@ -1510,6 +1519,9 @@ int hci_register_dev(struct hci_dev *hdev)
setup_timer(&hdev->adv_timer, hci_clear_adv_cache,
(unsigned long) hdev);
+ setup_timer(&hdev->le_scan_timer, hci_disable_le_scan,
+ (unsigned long) hdev);
+
INIT_WORK(&hdev->power_on, hci_power_on);
INIT_WORK(&hdev->power_off, hci_power_off);
setup_timer(&hdev->off_timer, hci_auto_off, (unsigned long) hdev);
@@ -1591,6 +1603,7 @@ int hci_unregister_dev(struct hci_dev *hdev)
hci_del_off_timer(hdev);
del_timer(&hdev->adv_timer);
+ del_timer(&hdev->le_scan_timer);
destroy_workqueue(hdev->workqueue);
--
1.7.4.1
Devices found during LE scan should be reported to userspace through
mgmt_device_found events.
Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/hci_event.c | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 4f85669..a4f0929 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2840,6 +2840,7 @@ static inline void hci_le_adv_report_evt(struct hci_dev *hdev,
{
struct hci_ev_le_advertising_info *ev;
u8 num_reports;
+ s8 rssi;
num_reports = skb->data[0];
ev = (void *) &skb->data[1];
@@ -2848,9 +2849,18 @@ static inline void hci_le_adv_report_evt(struct hci_dev *hdev,
hci_add_adv_entry(hdev, ev);
+ rssi = ev->data[ev->length];
+ mgmt_device_found(hdev->id, &ev->bdaddr, NULL, rssi, ev->data,
+ ev->length);
+
while (--num_reports) {
ev = (void *) (ev->data + ev->length + 1);
+
hci_add_adv_entry(hdev, ev);
+
+ rssi = ev->data[ev->length];
+ mgmt_device_found(hdev->id, &ev->bdaddr, NULL, rssi, ev->data,
+ ev->length);
}
hci_dev_unlock(hdev);
--
1.7.4.1
This patch adds a new parameter to mgmt_device_found() to inform
the length of 'eir' pointer.
EIR data from LE advertising report event doesn't have a fixed length
as EIR data from extended inquiry result event does. We needed to
change mgmt_device_found() so it copies 'eir_len' bytes instead of
HCI_MAX_EIR_LENGTH.
Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci_core.h | 2 +-
net/bluetooth/hci_event.c | 9 +++++----
net/bluetooth/mgmt.c | 7 +++++--
3 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 0d2e703..1ca4e60 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -855,7 +855,7 @@ int mgmt_set_local_name_complete(u16 index, u8 *name, u8 status);
int mgmt_read_local_oob_data_reply_complete(u16 index, u8 *hash, u8 *randomizer,
u8 status);
int mgmt_device_found(u16 index, bdaddr_t *bdaddr, u8 *dev_class, s8 rssi,
- u8 *eir);
+ u8 *eir, u8 eir_len);
int mgmt_remote_name(u16 index, bdaddr_t *bdaddr, u8 *name);
int mgmt_discovering(u16 index, u8 discovering);
int mgmt_start_discovery_complete(u16 index);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index c75211c..4f85669 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1393,7 +1393,7 @@ static inline void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *
data.ssp_mode = 0x00;
hci_inquiry_cache_update(hdev, &data);
mgmt_device_found(hdev->id, &info->bdaddr, info->dev_class, 0,
- NULL);
+ NULL, 0);
}
hci_dev_unlock(hdev);
@@ -2390,7 +2390,7 @@ static inline void hci_inquiry_result_with_rssi_evt(struct hci_dev *hdev, struct
hci_inquiry_cache_update(hdev, &data);
mgmt_device_found(hdev->id, &info->bdaddr,
info->dev_class, info->rssi,
- NULL);
+ NULL, 0);
}
} else {
struct inquiry_info_with_rssi *info = (void *) (skb->data + 1);
@@ -2407,7 +2407,7 @@ static inline void hci_inquiry_result_with_rssi_evt(struct hci_dev *hdev, struct
hci_inquiry_cache_update(hdev, &data);
mgmt_device_found(hdev->id, &info->bdaddr,
info->dev_class, info->rssi,
- NULL);
+ NULL, 0);
}
}
@@ -2549,7 +2549,8 @@ static inline void hci_extended_inquiry_result_evt(struct hci_dev *hdev, struct
data.ssp_mode = 0x01;
hci_inquiry_cache_update(hdev, &data);
mgmt_device_found(hdev->id, &info->bdaddr, info->dev_class,
- info->rssi, info->data);
+ info->rssi, info->data,
+ sizeof(info->data));
}
hci_dev_unlock(hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 796edd7..c0d3321 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2330,17 +2330,20 @@ int mgmt_read_local_oob_data_reply_complete(u16 index, u8 *hash, u8 *randomizer,
}
int mgmt_device_found(u16 index, bdaddr_t *bdaddr, u8 *dev_class, s8 rssi,
- u8 *eir)
+ u8 *eir, u8 eir_len)
{
struct mgmt_ev_device_found ev;
+ if (eir_len > sizeof(ev.eir))
+ return -EINVAL;
+
memset(&ev, 0, sizeof(ev));
bacpy(&ev.bdaddr, bdaddr);
ev.rssi = rssi;
if (eir)
- memcpy(ev.eir, eir, sizeof(ev.eir));
+ memcpy(ev.eir, eir, eir_len);
if (dev_class)
memcpy(ev.dev_class, dev_class, sizeof(ev.dev_class));
--
1.7.4.1
The mgmt_device_found event will be used to report LE devices found
during discovery procedure. Since LE advertising reports events
doesn't have class of device information, we need to check if
'dev_class' is not NULL before copying it.
Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/mgmt.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index dcfb466..796edd7 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2337,12 +2337,14 @@ int mgmt_device_found(u16 index, bdaddr_t *bdaddr, u8 *dev_class, s8 rssi,
memset(&ev, 0, sizeof(ev));
bacpy(&ev.bdaddr, bdaddr);
- memcpy(ev.dev_class, dev_class, sizeof(ev.dev_class));
ev.rssi = rssi;
if (eir)
memcpy(ev.eir, eir, sizeof(ev.eir));
+ if (dev_class)
+ memcpy(ev.dev_class, dev_class, sizeof(ev.dev_class));
+
return mgmt_event(MGMT_EV_DEVICE_FOUND, index, &ev, sizeof(ev), NULL);
}
--
1.7.4.1
This patch prepares start_discovery() to support LE-Only and BR/EDR/LE
devices discovery procedures (BR/EDR devices are already supported).
Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci.h | 1 +
include/net/bluetooth/hci_core.h | 1 +
net/bluetooth/mgmt.c | 37 ++++++++++++++++++++++++++++++++++++-
3 files changed, 38 insertions(+), 1 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index be30aab..653daec 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -202,6 +202,7 @@ enum {
#define LMP_EV4 0x01
#define LMP_EV5 0x02
+#define LMP_NO_BREDR 0x20
#define LMP_LE 0x40
#define LMP_SNIFF_SUBR 0x02
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 1ff59f2..0d2e703 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -597,6 +597,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
#define lmp_esco_capable(dev) ((dev)->features[3] & LMP_ESCO)
#define lmp_ssp_capable(dev) ((dev)->features[6] & LMP_SIMPLE_PAIR)
#define lmp_no_flush_capable(dev) ((dev)->features[6] & LMP_NO_FLUSH)
+#define lmp_bredr_capable(dev) (!((dev)->features[4] & LMP_NO_BREDR))
#define lmp_le_capable(dev) ((dev)->features[4] & LMP_LE)
/* ----- Extended LMP capabilities ----- */
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index bbb0daa..dcfb466 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -32,6 +32,15 @@
#define MGMT_VERSION 0
#define MGMT_REVISION 1
+enum bt_device_type {
+ BREDR_ONLY,
+ LE_ONLY,
+ BREDR_LE,
+ UNKNOWN,
+};
+
+#define BREDR_ONLY_INQ_LENGTH 0x08 /* TGAP(100) */
+
struct pending_cmd {
struct list_head list;
__u16 opcode;
@@ -1628,10 +1637,23 @@ static int do_inquiry(struct hci_dev *hdev, __u8 inq_length)
return hci_send_cmd(hdev, HCI_OP_INQUIRY, sizeof(cp), &cp);
}
+static int get_device_type(struct hci_dev *hdev)
+{
+ if (lmp_bredr_capable(hdev) && lmp_host_le_capable(hdev))
+ return BREDR_LE;
+ else if (lmp_host_le_capable(hdev))
+ return LE_ONLY;
+ else if (lmp_bredr_capable(hdev))
+ return BREDR_ONLY;
+ else
+ return UNKNOWN;
+}
+
static int start_discovery(struct sock *sk, u16 index)
{
struct pending_cmd *cmd;
struct hci_dev *hdev;
+ int dev_type;
int err;
BT_DBG("hci%u", index);
@@ -1654,7 +1676,20 @@ static int start_discovery(struct sock *sk, u16 index)
goto failed;
}
- err = do_inquiry(hdev, 0x08);
+ dev_type = get_device_type(hdev);
+
+ switch (dev_type) {
+ case BREDR_ONLY:
+ err = do_inquiry(hdev, BREDR_ONLY_INQ_LENGTH);
+ break;
+ case LE_ONLY:
+ case BREDR_LE:
+ err = -ENOSYS;
+ break;
+ default:
+ err = -EINVAL;
+ }
+
if (err < 0)
mgmt_pending_remove(cmd);
--
1.7.4.1
According to the Bluetooth spec, the inquiry cancel command should
only be issued after the inquiry command has been issued, a command
status event has been received for the inquiry command, and before
the inquiry complete event occurs.
As HCI_INQUIRY flag is only set just after an inquiry command status
event occurs and it is cleared just after an inquiry complete event
occurs, the inquiry cancel command should be issued only if HCI_INQUIRY
flag is set.
This spec constraint raises two possible race condition we must handle.
1. A MGMT_OP_STOP_DISCOVERY command is issued just after a
MGMT_OP_START_DISCOVERY. The controller didn't send the inquiry
command status yet therefore the HCI_INQUIRY flag is not set. Thus,
stop_discovery() will send no inquiry cancel command and the
discovery procedure won't be stopped. This race is addressed by
checking for pending MGMT_OP_STOP_DISCOVERY command in inquiry
command status event handler.
2. While the MGMT_OP_STOP_DISCOVERY is being processed the controller
sends the inquiry complete event and the HCI_INQUIRY flag is
cleared. stop_discovery() will add a pending MGMT_OP_STOP_DISCOVERY
command which won't be removed since there is no ongoing discovery.
This race is addressed by synchronizing stop_discovery and inquiry
complete event handler threads.
Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci_core.h | 2 ++
net/bluetooth/hci_event.c | 10 ++++++++++
net/bluetooth/mgmt.c | 25 ++++++++++++++++++++++++-
3 files changed, 36 insertions(+), 1 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 0ccd724..1ff59f2 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -861,6 +861,8 @@ int mgmt_start_discovery_complete(u16 index);
int mgmt_start_discovery_failed(u16 index, u8 status);
int mgmt_stop_discovery_complete(u16 index);
int mgmt_stop_discovery_failed(u16 index, u8 status);
+int mgmt_has_pending_stop_discov(u16 index);
+int mgmt_cancel_discovery(u16 index);
/* HCI info for socket */
#define hci_pi(sk) ((struct hci_pinfo *) sk)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index ea9e105..c75211c 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -963,6 +963,11 @@ static inline void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
set_bit(HCI_INQUIRY, &hdev->flags);
+ if (mgmt_has_pending_stop_discov(hdev->id)) {
+ mgmt_cancel_discovery(hdev->id);
+ return;
+ }
+
mgmt_discovering(hdev->id, 1);
}
@@ -1356,7 +1361,12 @@ static inline void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff
hci_req_complete(hdev, HCI_OP_INQUIRY, status);
mgmt_discovering(hdev->id, 0);
+
+ hci_dev_lock(hdev);
+
mgmt_start_discovery_complete(hdev->id);
+
+ hci_dev_unlock(hdev);
}
static inline void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *skb)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index e43940e..bbb0daa 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1670,6 +1670,21 @@ static int cancel_inquiry(struct hci_dev *hdev)
return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
}
+int mgmt_cancel_discovery(u16 index)
+{
+ struct hci_dev *hdev;
+ int res = 0;
+
+ hdev = hci_dev_get(index);
+
+ if (test_bit(HCI_INQUIRY, &hdev->flags))
+ res = cancel_inquiry(hdev);
+
+ hci_dev_put(hdev);
+
+ return res;
+}
+
static int stop_discovery(struct sock *sk, u16 index)
{
struct hci_dev *hdev;
@@ -1701,7 +1716,7 @@ static int stop_discovery(struct sock *sk, u16 index)
goto failed;
}
- err = cancel_inquiry(hdev);
+ err = mgmt_cancel_discovery(index);
if (err < 0)
mgmt_pending_remove(cmd);
@@ -2393,3 +2408,11 @@ int mgmt_stop_discovery_failed(u16 index, u8 status)
return err;
}
+
+int mgmt_has_pending_stop_discov(u16 index)
+{
+ if (mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, index))
+ return 1;
+
+ return 0;
+}
--
1.7.4.1
This patch moves all cancel inquiry code from stop_discovery() to
a new function called cancel_inquiry().
Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/mgmt.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 7ea8f95..e43940e 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1665,6 +1665,11 @@ failed:
return err;
}
+static int cancel_inquiry(struct hci_dev *hdev)
+{
+ return hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
+}
+
static int stop_discovery(struct sock *sk, u16 index)
{
struct hci_dev *hdev;
@@ -1696,7 +1701,7 @@ static int stop_discovery(struct sock *sk, u16 index)
goto failed;
}
- err = hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
+ err = cancel_inquiry(hdev);
if (err < 0)
mgmt_pending_remove(cmd);
--
1.7.4.1
This patch moves all inquiry code from start_discovery() to a new
function called do_inquiry().
Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/mgmt.c | 19 ++++++++++++-------
1 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index d673ea3..7ea8f95 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1616,10 +1616,20 @@ static int remove_remote_oob_data(struct sock *sk, u16 index,
return err;
}
-static int start_discovery(struct sock *sk, u16 index)
+static int do_inquiry(struct hci_dev *hdev, __u8 inq_length)
{
u8 lap[3] = { 0x33, 0x8b, 0x9e };
struct hci_cp_inquiry cp;
+
+ memset(&cp, 0, sizeof(cp));
+ memcpy(&cp.lap, lap, 3);
+ cp.length = inq_length;
+
+ return hci_send_cmd(hdev, HCI_OP_INQUIRY, sizeof(cp), &cp);
+}
+
+static int start_discovery(struct sock *sk, u16 index)
+{
struct pending_cmd *cmd;
struct hci_dev *hdev;
int err;
@@ -1644,12 +1654,7 @@ static int start_discovery(struct sock *sk, u16 index)
goto failed;
}
- memset(&cp, 0, sizeof(cp));
- memcpy(&cp.lap, lap, 3);
- cp.length = 0x08;
- cp.num_rsp = 0x00;
-
- err = hci_send_cmd(hdev, HCI_OP_INQUIRY, sizeof(cp), &cp);
+ err = do_inquiry(hdev, 0x08);
if (err < 0)
mgmt_pending_remove(cmd);
--
1.7.4.1
This patch adds extra checks in stop_discovery().
The MGMT_OP_STOP_DISCOVERY command should be executed if the device
is running the discovery procedure. So, if there is no discovery
procedure running then EINVAL command status should be returned.
Also, if a MGMT_OP_STOP_DISCOVERY command has been already issued
then EINPROGRESS command status should returned.
Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/mgmt.c | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 83693ac..d673ea3 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1674,6 +1674,17 @@ static int stop_discovery(struct sock *sk, u16 index)
hci_dev_lock_bh(hdev);
+ if (!mgmt_pending_find(MGMT_OP_START_DISCOVERY, index)) {
+ err = cmd_status(sk, index, MGMT_OP_STOP_DISCOVERY, EINVAL);
+ goto failed;
+ }
+
+ if (mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, index)) {
+ err = cmd_status(sk, index, MGMT_OP_STOP_DISCOVERY,
+ EINPROGRESS);
+ goto failed;
+ }
+
cmd = mgmt_pending_add(sk, MGMT_OP_STOP_DISCOVERY, index, NULL, 0);
if (!cmd) {
err = -ENOMEM;
--
1.7.4.1
If discovery procedure is already running then EINPROGRESS command
status should be returned.
Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/mgmt.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 6102648..83693ac 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1632,6 +1632,12 @@ static int start_discovery(struct sock *sk, u16 index)
hci_dev_lock_bh(hdev);
+ if (mgmt_pending_find(MGMT_OP_START_DISCOVERY, index)) {
+ err = cmd_status(sk, index, MGMT_OP_START_DISCOVERY,
+ EINPROGRESS);
+ goto failed;
+ }
+
cmd = mgmt_pending_add(sk, MGMT_OP_START_DISCOVERY, index, NULL, 0);
if (!cmd) {
err = -ENOMEM;
--
1.7.4.1
Discovery commands are added to the pending list but they aren't
removed.
This patch removes MGMT_OP_START_DISCOVERY and MGMT_OP_STOP_DISCOVERY
commands from pending command list when they terminate.
Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/hci_event.c | 14 +++++++++++++-
1 files changed, 13 insertions(+), 1 deletions(-)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index cf0efe5..ea9e105 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -55,8 +55,10 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
BT_DBG("%s status 0x%x", hdev->name, status);
- if (status)
+ if (status) {
+ mgmt_stop_discovery_failed(hdev->id, bt_to_errno(status));
return;
+ }
clear_bit(HCI_INQUIRY, &hdev->flags);
@@ -65,6 +67,7 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
hci_conn_check_pending(hdev);
mgmt_discovering(hdev->id, 0);
+ mgmt_stop_discovery_complete(hdev->id);
}
static void hci_cc_exit_periodic_inq(struct hci_dev *hdev, struct sk_buff *skb)
@@ -952,6 +955,9 @@ static inline void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
if (status) {
hci_req_complete(hdev, HCI_OP_INQUIRY, status);
hci_conn_check_pending(hdev);
+
+ mgmt_start_discovery_failed(hdev->id, bt_to_errno(status));
+
return;
}
@@ -1340,11 +1346,17 @@ static inline void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff
if (!test_bit(HCI_INQUIRY, &hdev->flags))
return;
+ if (status) {
+ mgmt_start_discovery_failed(hdev->id, bt_to_errno(status));
+ return;
+ }
+
clear_bit(HCI_INQUIRY, &hdev->flags);
hci_req_complete(hdev, HCI_OP_INQUIRY, status);
mgmt_discovering(hdev->id, 0);
+ mgmt_start_discovery_complete(hdev->id);
}
static inline void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *skb)
--
1.7.4.1
These functions remove pending commands and send command complete/
status events.
Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci_core.h | 4 ++
net/bluetooth/mgmt.c | 80 ++++++++++++++++++++++++++++++++++++++
2 files changed, 84 insertions(+), 0 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index c41e275..0ccd724 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -857,6 +857,10 @@ int mgmt_device_found(u16 index, bdaddr_t *bdaddr, u8 *dev_class, s8 rssi,
u8 *eir);
int mgmt_remote_name(u16 index, bdaddr_t *bdaddr, u8 *name);
int mgmt_discovering(u16 index, u8 discovering);
+int mgmt_start_discovery_complete(u16 index);
+int mgmt_start_discovery_failed(u16 index, u8 status);
+int mgmt_stop_discovery_complete(u16 index);
+int mgmt_stop_discovery_failed(u16 index, u8 status);
/* HCI info for socket */
#define hci_pi(sk) ((struct hci_pinfo *) sk)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 53e109e..6102648 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2286,3 +2286,83 @@ int mgmt_discovering(u16 index, u8 discovering)
return mgmt_event(MGMT_EV_DISCOVERING, index, &discovering,
sizeof(discovering), NULL);
}
+
+int mgmt_start_discovery_complete(u16 index)
+{
+ struct pending_cmd *cmd;
+ int err;
+
+ cmd = mgmt_pending_find(MGMT_OP_START_DISCOVERY, index);
+ if (!cmd)
+ return -ENOENT;
+
+ err = cmd_complete(cmd->sk, index, MGMT_OP_START_DISCOVERY, NULL, 0);
+
+ mgmt_pending_remove(cmd);
+
+ cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, index);
+ if (cmd) {
+ cmd_status(cmd->sk, index, MGMT_OP_STOP_DISCOVERY, EPERM);
+ mgmt_pending_remove(cmd);
+ }
+
+ return err;
+}
+
+int mgmt_start_discovery_failed(u16 index, u8 status)
+{
+ struct pending_cmd *cmd;
+ int err;
+
+ cmd = mgmt_pending_find(MGMT_OP_START_DISCOVERY, index);
+ if (!cmd)
+ return -ENOENT;
+
+ err = cmd_status(cmd->sk, index, MGMT_OP_START_DISCOVERY, status);
+
+ mgmt_pending_remove(cmd);
+
+ cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, index);
+ if (cmd) {
+ cmd_status(cmd->sk, index, MGMT_OP_STOP_DISCOVERY, EPERM);
+ mgmt_pending_remove(cmd);
+ }
+
+ return err;
+}
+
+int mgmt_stop_discovery_complete(u16 index)
+{
+ struct pending_cmd *cmd;
+ int err;
+
+ cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, index);
+ if (!cmd)
+ return -ENOENT;
+
+ err = cmd_complete(cmd->sk, index, MGMT_OP_STOP_DISCOVERY, NULL, 0);
+
+ mgmt_pending_remove(cmd);
+
+ cmd = mgmt_pending_find(MGMT_OP_START_DISCOVERY, index);
+ if (cmd)
+ mgmt_pending_remove(cmd);
+
+ return err;
+}
+
+int mgmt_stop_discovery_failed(u16 index, u8 status)
+{
+ struct pending_cmd *cmd;
+ int err;
+
+ cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, index);
+ if (!cmd)
+ return -ENOENT;
+
+ err = cmd_status(cmd->sk, index, MGMT_OP_STOP_DISCOVERY, status);
+
+ mgmt_pending_remove(cmd);
+
+ return err;
+}
--
1.7.4.1
By using periodic inquiry command we're not able to detect correctly
when the controller has started inquiry.
Today we have this workaround in inquiry result event handler to set
the HCI_INQUIRY flag when it sees the first inquiry result event.
This workaround isn't enough because the device may be performing
an inquiry but the HCI_INQUIRY flag is not set. For instance, if
there is no device in range, no inquiry result event is generated,
consequently, the HCI_INQUIRY flags isn't set when it should so.
We rely on HCI_INQUIRY flag to implement the discovery procedure
properly. So, as we aren't able to clear/set the HCI_INQUIRY flag in
a reliable manner, periodic inquiry events shouldn't change the
HCI_INQUIRY flag. In future, if needed, we might add a new flag (e.g.
HCI_PINQUIRY) to know if the controller is performing periodic
inquiry.
Thus, due to that issue and in order to keep compatibility with
userspace, periodic inquiry events shouldn't send mgmt discovering
events.
Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/hci_event.c | 46 ++++++++++++--------------------------------
1 files changed, 13 insertions(+), 33 deletions(-)
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index a40170e..cf0efe5 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -58,13 +58,13 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
if (status)
return;
- if (test_bit(HCI_MGMT, &hdev->flags) &&
- test_and_clear_bit(HCI_INQUIRY, &hdev->flags))
- mgmt_discovering(hdev->id, 0);
+ clear_bit(HCI_INQUIRY, &hdev->flags);
hci_req_complete(hdev, HCI_OP_INQUIRY_CANCEL, status);
hci_conn_check_pending(hdev);
+
+ mgmt_discovering(hdev->id, 0);
}
static void hci_cc_exit_periodic_inq(struct hci_dev *hdev, struct sk_buff *skb)
@@ -76,10 +76,6 @@ static void hci_cc_exit_periodic_inq(struct hci_dev *hdev, struct sk_buff *skb)
if (status)
return;
- if (test_bit(HCI_MGMT, &hdev->flags) &&
- test_and_clear_bit(HCI_INQUIRY, &hdev->flags))
- mgmt_discovering(hdev->id, 0);
-
hci_conn_check_pending(hdev);
}
@@ -959,10 +955,9 @@ static inline void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
return;
}
- if (test_bit(HCI_MGMT, &hdev->flags) &&
- !test_and_set_bit(HCI_INQUIRY,
- &hdev->flags))
- mgmt_discovering(hdev->id, 1);
+ set_bit(HCI_INQUIRY, &hdev->flags);
+
+ mgmt_discovering(hdev->id, 1);
}
static inline void hci_cs_create_conn(struct hci_dev *hdev, __u8 status)
@@ -1340,13 +1335,16 @@ static inline void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_buff
BT_DBG("%s status %d", hdev->name, status);
- if (test_bit(HCI_MGMT, &hdev->flags) &&
- test_and_clear_bit(HCI_INQUIRY, &hdev->flags))
- mgmt_discovering(hdev->id, 0);
+ hci_conn_check_pending(hdev);
+
+ if (!test_bit(HCI_INQUIRY, &hdev->flags))
+ return;
+
+ clear_bit(HCI_INQUIRY, &hdev->flags);
hci_req_complete(hdev, HCI_OP_INQUIRY, status);
- hci_conn_check_pending(hdev);
+ mgmt_discovering(hdev->id, 0);
}
static inline void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *skb)
@@ -1362,12 +1360,6 @@ static inline void hci_inquiry_result_evt(struct hci_dev *hdev, struct sk_buff *
hci_dev_lock(hdev);
- if (!test_and_set_bit(HCI_INQUIRY, &hdev->flags)) {
-
- if (test_bit(HCI_MGMT, &hdev->flags))
- mgmt_discovering(hdev->id, 1);
- }
-
for (; num_rsp; num_rsp--, info++) {
bacpy(&data.bdaddr, &info->bdaddr);
data.pscan_rep_mode = info->pscan_rep_mode;
@@ -2360,12 +2352,6 @@ static inline void hci_inquiry_result_with_rssi_evt(struct hci_dev *hdev, struct
hci_dev_lock(hdev);
- if (!test_and_set_bit(HCI_INQUIRY, &hdev->flags)) {
-
- if (test_bit(HCI_MGMT, &hdev->flags))
- mgmt_discovering(hdev->id, 1);
- }
-
if ((skb->len - 1) / num_rsp != sizeof(struct inquiry_info_with_rssi)) {
struct inquiry_info_with_rssi_and_pscan_mode *info;
info = (void *) (skb->data + 1);
@@ -2528,12 +2514,6 @@ static inline void hci_extended_inquiry_result_evt(struct hci_dev *hdev, struct
if (!num_rsp)
return;
- if (!test_and_set_bit(HCI_INQUIRY, &hdev->flags)) {
-
- if (test_bit(HCI_MGMT, &hdev->flags))
- mgmt_discovering(hdev->id, 1);
- }
-
hci_dev_lock(hdev);
for (; num_rsp; num_rsp--, info++) {
--
1.7.4.1
Hi Marcel,
On Aug 10, 2011, at 10:52 AM, Marcel Holtmann wrote:
> Hi Andre,
>
>> This patch adds support for LE-Only discovery procedure through
>> management interface.
>>
>> A new flag (HCI_LE_SCAN) was created to inform if the controller is
>> performing LE scan. The HCI_LE_SCAN flag is set/cleared when the
>> controller starts/stops scanning.
>>
>> Signed-off-by: Andre Guedes <[email protected]>
>> ---
>> include/net/bluetooth/hci.h | 2 ++
>> net/bluetooth/hci_event.c | 39 +++++++++++++++++++++++++++++++++
>> +++---
>> net/bluetooth/mgmt.c | 5 +++++
>> 3 files changed, 43 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/
>> hci.h
>> index fb40388..c4fdeeb 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -86,6 +86,8 @@ enum {
>> HCI_DEBUG_KEYS,
>>
>> HCI_RESET,
>> +
>> + HCI_LE_SCAN,
>> };
>
> I am really against adding any new flags here. This is a public API
> and
> a horrible one actually.
>
> We need to have these states internal and stop adding more flags to
> this
> public API.
The HCI_LE_SCAN flag is really device/controller related as well as
HCI_INQURY flag is. They both have similar meaning and using. Besides,
the userspace (hciconfig) might be interested in checking this flag to
know if the controller is carrying out the LE scan (just like it does
with HCI_INQUIRY flag).
So, may you consider we keep this flag here?
Thanks,
Andre.
Hi Marcel/Lizardo,
On Aug 10, 2011, at 9:26 PM, Marcel Holtmann wrote:
> Hi Anderson,
>
>>> The advertising report event has the 'Length' field to inform the
>>> 'Data' field length, so I believe it has a variable length.
>>> According to its description, the 'Length' field may vary from 0x00
>>> to 0x1F (31) bytes.
>>
>> You are right. Although the section 11 gives the impression the size
>> is always 31, this is not what happens on actual hardware, which
>> usually sends only the significant bytes (and the length is know from
>> the "Length" field.
>>
>>> The only drawback I see so far is copying extra ~200 bytes each time
>>> we get a advertising report data.
>>
>> I agree. If this event is being sent on each adv. data report event,
>> it will be more than 6 times the amount of data (with non-significant
>> bytes containing only zeroes) sent to userspace.
>
> if we wanna save bytes copied and send to userspace, then we might
> even
> fix this for BR/EDR. Since while it is fixed there only a fraction is
> used there most of the times.
IMO changing the struct device_found is another issue (this would
require userspace changes too).
The point in adding the eir_len parameter is to avoid:
* preparing (memset and copy EIR from adv report) a 240-bytes
temporary buffer to pass to mgmt_device_found()
* to memcpy() ~200 useless bytes to ev.eir in mgmt_device_found()
For each advertising data we gather from the LE scan (which can be
a lot) we would need to do that.
BR,
Andre.
Hi Anderson,
> > The advertising report event has the 'Length' field to inform the
> > 'Data' field length, so I believe it has a variable length.
> > According to its description, the 'Length' field may vary from 0x00
> > to 0x1F (31) bytes.
>
> You are right. Although the section 11 gives the impression the size
> is always 31, this is not what happens on actual hardware, which
> usually sends only the significant bytes (and the length is know from
> the "Length" field.
>
> > The only drawback I see so far is copying extra ~200 bytes each time
> > we get a advertising report data.
>
> I agree. If this event is being sent on each adv. data report event,
> it will be more than 6 times the amount of data (with non-significant
> bytes containing only zeroes) sent to userspace.
if we wanna save bytes copied and send to userspace, then we might even
fix this for BR/EDR. Since while it is fixed there only a fraction is
used there most of the times.
Regards
Marcel
Hi Andre,
<snip>
> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/
> >> bluetooth/hci_core.h
> >> index 1ff59f2..0d2e703 100644
> >> --- a/include/net/bluetooth/hci_core.h
> >> +++ b/include/net/bluetooth/hci_core.h
> >> @@ -597,6 +597,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
> >> #define lmp_esco_capable(dev) ((dev)->features[3] & LMP_ESCO)
> >> #define lmp_ssp_capable(dev) ((dev)->features[6] &
> >> LMP_SIMPLE_PAIR)
> >> #define lmp_no_flush_capable(dev) ((dev)->features[6] &
> >> LMP_NO_FLUSH)
> >> +#define lmp_bredr_capable(dev) (!((dev)->features[4] &
> >> LMP_NO_BREDR))
> >
> > I don't think this is a good idea. You keep forgetting if you actually
> > have LE switched on or not.
> >
> > I think we should keep it like this and just keep a global hci_dev
> > state
> > which discovery procedure to use. Depending on if the device is just
> > really LE-Only, it is dual-stack, but LE got switched off (we will
> > need
> > this eventually for testing) or it is just only BR/EDR.
>
> I agree. What really matters for the discovery procedure is the
> operation mode not the device type. The term "device type" was
> misused here.
>
> About the global hci_dev state, IMO we may not need it. By looking
> at the controller's LMP features we are able to infer what is the
> controller's operation mode.
you need to look at the extended features page 1, but yes, if you have
that available, the it might be as simple as have the global state.
It really depends on how often you have to check. Maybe it is worth to
just have a simple operation mode variable compared to always have to
access multiple bits in the features array. Depends on how often you
need to use it.
> >> #define lmp_le_capable(dev) ((dev)->features[4] & LMP_LE)
> >>
> >> /* ----- Extended LMP capabilities ----- */
> >> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> >> index bbb0daa..dcfb466 100644
> >> --- a/net/bluetooth/mgmt.c
> >> +++ b/net/bluetooth/mgmt.c
> >> @@ -32,6 +32,15 @@
> >> #define MGMT_VERSION 0
> >> #define MGMT_REVISION 1
> >>
> >> +enum bt_device_type {
> >> + BREDR_ONLY,
> >> + LE_ONLY,
> >> + BREDR_LE,
> >> + UNKNOWN,
> >> +};
> >
> > What is this for? We essentially have a local device capabilities
> > and an
> > operation mode. They are both different. We do not have a device type.
>
> I'll change this. I may call this bt_operation_mode or something.
Call it bt_opermode or similar. Something shorter than some long crazy
name that takes a lot of space at least ;)
Also remove the UNKNOWN thing. That is useless. The devices is either
BREDR or LE_ONLY anyway. And worst case it is really BREDR. That has
been the default for a long time now.
And while at it just call it BREDR. And prefix this with
BT_OPERMODE_BREDR or so.
Regards
Marcel
Hi,
On Wed, Aug 10, 2011 at 3:51 PM, Andre Guedes
<[email protected]> wrote:
> The advertising report event has the 'Length' field to inform the
> 'Data' field length, so I believe it has a variable length.
> According to its description, the 'Length' field may vary from 0x00
> to 0x1F (31) bytes.
You are right. Although the section 11 gives the impression the size
is always 31, this is not what happens on actual hardware, which
usually sends only the significant bytes (and the length is know from
the "Length" field.
> The only drawback I see so far is copying extra ~200 bytes each time
> we get a advertising report data.
I agree. If this event is being sent on each adv. data report event,
it will be more than 6 times the amount of data (with non-significant
bytes containing only zeroes) sent to userspace.
Now one idea: if we guarantee that the EIR/Adv. data field will always
be the last one, can we assume all remaining bytes on the event are
EIR/Adv info? I.e. the equivalent in C of:
struct device_found {
bdaddr_t addr;
uint8_t class_of_device[3];
int8_t rssi;
uint8_t eir[];
};
Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
Hi Marcel/Lizardo,
On Aug 10, 2011, at 12:17 PM, Marcel Holtmann wrote:
> Hi Anderson,
>
>>>> This patch adds a new parameter to mgmt_device_found() to inform
>>>> the length of 'eir' pointer.
>>>>
>>>> EIR data from LE advertising report event doesn't have a fixed
>>>> length
>>>> as EIR data from extended inquiry result event does. We needed to
>>>> change mgmt_device_found() so it copies 'eir_len' bytes instead of
>>>> HCI_MAX_EIR_LENGTH.
>>>
>>> what is the max EIR length for LE? Is it more than with BR/EDR. Or
>>> is it
>>> just variable length?
>>
>> From page 1735:
>>
>> "The data consists of a significant part and a non-significant part.
>> [...] The non-significant part extends the Advertising and Scan
>> Response data to 31 octets and shall contain all-zero octets."
>>
>> So actually it is not variable length, it has 31 octets, with the
>> non-significant part zero padded. BR/EDR EIR, on the other hand is
>> 240
>> bytes (with zero padding for non-significant part as well)
>
> do we have a deep impact (memory wise) if we always pad this up to
> 240.
> If we do, then we might wanna be smarter here anyway.
The advertising report event has the 'Length' field to inform the
'Data' field length, so I believe it has a variable length.
According to its description, the 'Length' field may vary from 0x00
to 0x1F (31) bytes.
The only drawback I see so far is copying extra ~200 bytes each time
we get a advertising report data.
- Andre
Hi Marcel,
On Aug 10, 2011, at 10:48 AM, Marcel Holtmann wrote:
> Hi Andre,
>
>> This patch prepares start_discovery() to support LE-Only and BR/EDR/
>> LE
>> devices discovery procedures (BR/EDR devices are already supported).
>>
>> Signed-off-by: Andre Guedes <[email protected]>
>> ---
>> include/net/bluetooth/hci.h | 1 +
>> include/net/bluetooth/hci_core.h | 1 +
>> net/bluetooth/mgmt.c | 37 ++++++++++++++++++++++++++++
>> ++++++++-
>> 3 files changed, 38 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/
>> hci.h
>> index be30aab..653daec 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -202,6 +202,7 @@ enum {
>>
>> #define LMP_EV4 0x01
>> #define LMP_EV5 0x02
>> +#define LMP_NO_BREDR 0x20
>> #define LMP_LE 0x40
>>
>> #define LMP_SNIFF_SUBR 0x02
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/
>> bluetooth/hci_core.h
>> index 1ff59f2..0d2e703 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -597,6 +597,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
>> #define lmp_esco_capable(dev) ((dev)->features[3] & LMP_ESCO)
>> #define lmp_ssp_capable(dev) ((dev)->features[6] &
>> LMP_SIMPLE_PAIR)
>> #define lmp_no_flush_capable(dev) ((dev)->features[6] &
>> LMP_NO_FLUSH)
>> +#define lmp_bredr_capable(dev) (!((dev)->features[4] &
>> LMP_NO_BREDR))
>
> I don't think this is a good idea. You keep forgetting if you actually
> have LE switched on or not.
>
> I think we should keep it like this and just keep a global hci_dev
> state
> which discovery procedure to use. Depending on if the device is just
> really LE-Only, it is dual-stack, but LE got switched off (we will
> need
> this eventually for testing) or it is just only BR/EDR.
I agree. What really matters for the discovery procedure is the
operation mode not the device type. The term "device type" was
misused here.
About the global hci_dev state, IMO we may not need it. By looking
at the controller's LMP features we are able to infer what is the
controller's operation mode.
>> #define lmp_le_capable(dev) ((dev)->features[4] & LMP_LE)
>>
>> /* ----- Extended LMP capabilities ----- */
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index bbb0daa..dcfb466 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -32,6 +32,15 @@
>> #define MGMT_VERSION 0
>> #define MGMT_REVISION 1
>>
>> +enum bt_device_type {
>> + BREDR_ONLY,
>> + LE_ONLY,
>> + BREDR_LE,
>> + UNKNOWN,
>> +};
>
> What is this for? We essentially have a local device capabilities
> and an
> operation mode. They are both different. We do not have a device type.
I'll change this. I may call this bt_operation_mode or something.
>
>> +
>> +#define BREDR_ONLY_INQ_LENGTH 0x08 /* TGAP(100) */
>> +
>> struct pending_cmd {
>> struct list_head list;
>> __u16 opcode;
>> @@ -1628,10 +1637,23 @@ static int do_inquiry(struct hci_dev *hdev,
>> __u8 inq_length)
>> return hci_send_cmd(hdev, HCI_OP_INQUIRY, sizeof(cp), &cp);
>> }
>>
>> +static int get_device_type(struct hci_dev *hdev)
>> +{
>> + if (lmp_bredr_capable(hdev) && lmp_host_le_capable(hdev))
>> + return BREDR_LE;
>> + else if (lmp_host_le_capable(hdev))
>> + return LE_ONLY;
>> + else if (lmp_bredr_capable(hdev))
>> + return BREDR_ONLY;
>> + else
>> + return UNKNOWN;
>> +}
>> +
>> static int start_discovery(struct sock *sk, u16 index)
>> {
>> struct pending_cmd *cmd;
>> struct hci_dev *hdev;
>> + int dev_type;
>> int err;
>>
>> BT_DBG("hci%u", index);
>> @@ -1654,7 +1676,20 @@ static int start_discovery(struct sock *sk,
>> u16 index)
>> goto failed;
>> }
>>
>> - err = do_inquiry(hdev, 0x08);
>> + dev_type = get_device_type(hdev);
>> +
>> + switch (dev_type) {
>> + case BREDR_ONLY:
>> + err = do_inquiry(hdev, BREDR_ONLY_INQ_LENGTH);
>> + break;
>> + case LE_ONLY:
>> + case BREDR_LE:
>> + err = -ENOSYS;
>> + break;
>> + default:
>> + err = -EINVAL;
>> + }
>> +
>> if (err < 0)
>> mgmt_pending_remove(cmd);
>>
>
> As I said, device type is fundamentally wrong approach. You need to go
> for operation mode here.
I'll change this too.
Thanks,
Andre.
Hi Anderson,
> >> This patch adds a new parameter to mgmt_device_found() to inform
> >> the length of 'eir' pointer.
> >>
> >> EIR data from LE advertising report event doesn't have a fixed length
> >> as EIR data from extended inquiry result event does. We needed to
> >> change mgmt_device_found() so it copies 'eir_len' bytes instead of
> >> HCI_MAX_EIR_LENGTH.
> >
> > what is the max EIR length for LE? Is it more than with BR/EDR. Or is it
> > just variable length?
>
> From page 1735:
>
> "The data consists of a significant part and a non-significant part.
> [...] The non-significant part extends the Advertising and Scan
> Response data to 31 octets and shall contain all-zero octets."
>
> So actually it is not variable length, it has 31 octets, with the
> non-significant part zero padded. BR/EDR EIR, on the other hand is 240
> bytes (with zero padding for non-significant part as well)
do we have a deep impact (memory wise) if we always pad this up to 240.
If we do, then we might wanna be smarter here anyway.
Regards
Marcel
Hi Marcel/Andre,
On Wed, Aug 10, 2011 at 9:50 AM, Marcel Holtmann <[email protected]> wrote:
> Hi Andre,
>
>> This patch adds a new parameter to mgmt_device_found() to inform
>> the length of 'eir' pointer.
>>
>> EIR data from LE advertising report event doesn't have a fixed length
>> as EIR data from extended inquiry result event does. We needed to
>> change mgmt_device_found() so it copies 'eir_len' bytes instead of
>> HCI_MAX_EIR_LENGTH.
>
> what is the max EIR length for LE? Is it more than with BR/EDR. Or is it
> just variable length?
>From page 1735:
"The data consists of a significant part and a non-significant part.
[...] The non-significant part extends the Advertising and Scan
Response data to 31 octets and shall contain all-zero octets."
So actually it is not variable length, it has 31 octets, with the
non-significant part zero padded. BR/EDR EIR, on the other hand is 240
bytes (with zero padding for non-significant part as well)
Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
Hi Andre,
> This patch adds support for LE-Only discovery procedure through
> management interface.
>
> A new flag (HCI_LE_SCAN) was created to inform if the controller is
> performing LE scan. The HCI_LE_SCAN flag is set/cleared when the
> controller starts/stops scanning.
>
> Signed-off-by: Andre Guedes <[email protected]>
> ---
> include/net/bluetooth/hci.h | 2 ++
> net/bluetooth/hci_event.c | 39 ++++++++++++++++++++++++++++++++++++---
> net/bluetooth/mgmt.c | 5 +++++
> 3 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index fb40388..c4fdeeb 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -86,6 +86,8 @@ enum {
> HCI_DEBUG_KEYS,
>
> HCI_RESET,
> +
> + HCI_LE_SCAN,
> };
I am really against adding any new flags here. This is a public API and
a horrible one actually.
We need to have these states internal and stop adding more flags to this
public API.
Regards
Marcel
Hi Andre,
> This patch adds a new parameter to mgmt_device_found() to inform
> the length of 'eir' pointer.
>
> EIR data from LE advertising report event doesn't have a fixed length
> as EIR data from extended inquiry result event does. We needed to
> change mgmt_device_found() so it copies 'eir_len' bytes instead of
> HCI_MAX_EIR_LENGTH.
what is the max EIR length for LE? Is it more than with BR/EDR. Or is it
just variable length?
Regards
Marcel
Hi Andre,
> This patch prepares start_discovery() to support LE-Only and BR/EDR/LE
> devices discovery procedures (BR/EDR devices are already supported).
>
> Signed-off-by: Andre Guedes <[email protected]>
> ---
> include/net/bluetooth/hci.h | 1 +
> include/net/bluetooth/hci_core.h | 1 +
> net/bluetooth/mgmt.c | 37 ++++++++++++++++++++++++++++++++++++-
> 3 files changed, 38 insertions(+), 1 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index be30aab..653daec 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -202,6 +202,7 @@ enum {
>
> #define LMP_EV4 0x01
> #define LMP_EV5 0x02
> +#define LMP_NO_BREDR 0x20
> #define LMP_LE 0x40
>
> #define LMP_SNIFF_SUBR 0x02
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 1ff59f2..0d2e703 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -597,6 +597,7 @@ void hci_conn_del_sysfs(struct hci_conn *conn);
> #define lmp_esco_capable(dev) ((dev)->features[3] & LMP_ESCO)
> #define lmp_ssp_capable(dev) ((dev)->features[6] & LMP_SIMPLE_PAIR)
> #define lmp_no_flush_capable(dev) ((dev)->features[6] & LMP_NO_FLUSH)
> +#define lmp_bredr_capable(dev) (!((dev)->features[4] & LMP_NO_BREDR))
I don't think this is a good idea. You keep forgetting if you actually
have LE switched on or not.
I think we should keep it like this and just keep a global hci_dev state
which discovery procedure to use. Depending on if the device is just
really LE-Only, it is dual-stack, but LE got switched off (we will need
this eventually for testing) or it is just only BR/EDR.
> #define lmp_le_capable(dev) ((dev)->features[4] & LMP_LE)
>
> /* ----- Extended LMP capabilities ----- */
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index bbb0daa..dcfb466 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -32,6 +32,15 @@
> #define MGMT_VERSION 0
> #define MGMT_REVISION 1
>
> +enum bt_device_type {
> + BREDR_ONLY,
> + LE_ONLY,
> + BREDR_LE,
> + UNKNOWN,
> +};
What is this for? We essentially have a local device capabilities and an
operation mode. They are both different. We do not have a device type.
> +
> +#define BREDR_ONLY_INQ_LENGTH 0x08 /* TGAP(100) */
> +
> struct pending_cmd {
> struct list_head list;
> __u16 opcode;
> @@ -1628,10 +1637,23 @@ static int do_inquiry(struct hci_dev *hdev, __u8 inq_length)
> return hci_send_cmd(hdev, HCI_OP_INQUIRY, sizeof(cp), &cp);
> }
>
> +static int get_device_type(struct hci_dev *hdev)
> +{
> + if (lmp_bredr_capable(hdev) && lmp_host_le_capable(hdev))
> + return BREDR_LE;
> + else if (lmp_host_le_capable(hdev))
> + return LE_ONLY;
> + else if (lmp_bredr_capable(hdev))
> + return BREDR_ONLY;
> + else
> + return UNKNOWN;
> +}
> +
> static int start_discovery(struct sock *sk, u16 index)
> {
> struct pending_cmd *cmd;
> struct hci_dev *hdev;
> + int dev_type;
> int err;
>
> BT_DBG("hci%u", index);
> @@ -1654,7 +1676,20 @@ static int start_discovery(struct sock *sk, u16 index)
> goto failed;
> }
>
> - err = do_inquiry(hdev, 0x08);
> + dev_type = get_device_type(hdev);
> +
> + switch (dev_type) {
> + case BREDR_ONLY:
> + err = do_inquiry(hdev, BREDR_ONLY_INQ_LENGTH);
> + break;
> + case LE_ONLY:
> + case BREDR_LE:
> + err = -ENOSYS;
> + break;
> + default:
> + err = -EINVAL;
> + }
> +
> if (err < 0)
> mgmt_pending_remove(cmd);
>
As I said, device type is fundamentally wrong approach. You need to go
for operation mode here.
Regards
Marcel
Hi Marcel,
On Aug 10, 2011, at 9:24 PM, Marcel Holtmann wrote:
>>> I don't think this is a good idea. You keep forgetting if you actually
>>> have LE switched on or not.
>>>
>>> I think we should keep it like this and just keep a global hci_dev
>>> state
>>> which discovery procedure to use. Depending on if the device is just
>>> really LE-Only, it is dual-stack, but LE got switched off (we will
>>> need
>>> this eventually for testing) or it is just only BR/EDR.
>>
>> I agree. What really matters for the discovery procedure is the
>> operation mode not the device type. The term "device type" was
>> misused here.
>>
>> About the global hci_dev state, IMO we may not need it. By looking
>> at the controller's LMP features we are able to infer what is the
>> controller's operation mode.
>
> you need to look at the extended features page 1, but yes, if you have
> that available, the it might be as simple as have the global state.
>
> It really depends on how often you have to check. Maybe it is worth to
> just have a simple operation mode variable compared to always have to
> access multiple bits in the features array. Depends on how often you
> need to use it.
Yes, sure. Since we don't check these bits so often, we can check the
LMP features instead of having the global variable. This way we keep
it simple.
BR,
Andre.
Hi Marcel,
On Sep 5, 2011, at 6:03 AM, Marcel Holtmann wrote:
> Hi Andre,
>
>>>>> The advertising report event has the 'Length' field to inform the
>>>>> 'Data' field length, so I believe it has a variable length.
>>>>> According to its description, the 'Length' field may vary from 0x00
>>>>> to 0x1F (31) bytes.
>>>>
>>>> You are right. Although the section 11 gives the impression the size
>>>> is always 31, this is not what happens on actual hardware, which
>>>> usually sends only the significant bytes (and the length is know from
>>>> the "Length" field.
>>>>
>>>>> The only drawback I see so far is copying extra ~200 bytes each time
>>>>> we get a advertising report data.
>>>>
>>>> I agree. If this event is being sent on each adv. data report event,
>>>> it will be more than 6 times the amount of data (with non-significant
>>>> bytes containing only zeroes) sent to userspace.
>>>
>>> if we wanna save bytes copied and send to userspace, then we might
>>> even
>>> fix this for BR/EDR. Since while it is fixed there only a fraction is
>>> used there most of the times.
>>
>> IMO changing the struct device_found is another issue (this would
>> require userspace changes too).
>
> personally I break this one now, instead of later.
>
>> The point in adding the eir_len parameter is to avoid:
>> * preparing (memset and copy EIR from adv report) a 240-bytes
>> temporary buffer to pass to mgmt_device_found()
>> * to memcpy() ~200 useless bytes to ev.eir in mgmt_device_found()
>>
>> For each advertising data we gather from the LE scan (which can be
>> a lot) we would need to do that.
>
> And the same applies to BR/EDR actually. It is just that we never really
> encounter lot of inquiry results (except UPF), but it is the same
> problem that we end up copying data around for no good reason.
BR/EDR case is slightly different. The EIR field in Extended Inquiry
Result Event doesn't have variable length, it has 240-bytes fixed
length (see page 1012). So, we don't need to "prepare" any temporary
buffer to pass to mgmt_device_found().
> If we wanna optimize this, then we better do it for both. So we have the
> same handling towards userspace.
This patch doesn't optimize any kernel-land/user-land data transfer at all.
The optimization done here is in-kernel only. It avoids some extra
operations in hci_le_adv_report_evt() in order to send mgmt_device_found
events.
If we want to optimize kernel/user data transfer by sending only the
significant part of EIR in struct mgmt_ev_device_found we might need to:
- add a eir_length field to struct mgmt_ev_device_found (this implies
changing the MGMT API).
- replace the 'eir' field (240-byte buffer) by a u8 pointer in
struct mgmt_ev_device_found (this changes the MGMT API too).
- parser EIR in hci_extended_inquiry_result_evt() to get the length
of its significant part.
- do the proper handling of mgmt_device_found events in userspace.
Since this optimization will require some effort, it isn't related to
discovery support itself and we have other patches depending on this
discovery series, I'm afraid we can't work on this kernel/user data
transfer optimization right now. ASA we have mgmt interface stable and
LE main features implemented we might work on these optimizations.
So, I'll send the new version of discovery patch series soon with all
issues we've discussed.
Thanks,
Andre.
Hi Andre,
> >>> The advertising report event has the 'Length' field to inform the
> >>> 'Data' field length, so I believe it has a variable length.
> >>> According to its description, the 'Length' field may vary from 0x00
> >>> to 0x1F (31) bytes.
> >>
> >> You are right. Although the section 11 gives the impression the size
> >> is always 31, this is not what happens on actual hardware, which
> >> usually sends only the significant bytes (and the length is know from
> >> the "Length" field.
> >>
> >>> The only drawback I see so far is copying extra ~200 bytes each time
> >>> we get a advertising report data.
> >>
> >> I agree. If this event is being sent on each adv. data report event,
> >> it will be more than 6 times the amount of data (with non-significant
> >> bytes containing only zeroes) sent to userspace.
> >
> > if we wanna save bytes copied and send to userspace, then we might
> > even
> > fix this for BR/EDR. Since while it is fixed there only a fraction is
> > used there most of the times.
>
> IMO changing the struct device_found is another issue (this would
> require userspace changes too).
personally I break this one now, instead of later.
> The point in adding the eir_len parameter is to avoid:
> * preparing (memset and copy EIR from adv report) a 240-bytes
> temporary buffer to pass to mgmt_device_found()
> * to memcpy() ~200 useless bytes to ev.eir in mgmt_device_found()
>
> For each advertising data we gather from the LE scan (which can be
> a lot) we would need to do that.
And the same applies to BR/EDR actually. It is just that we never really
encounter lot of inquiry results (except UPF), but it is the same
problem that we end up copying data around for no good reason.
If we wanna optimize this, then we better do it for both. So we have the
same handling towards userspace.
Regards
Marcel
Hi Andre,
> >> This patch adds support for LE-Only discovery procedure through
> >> management interface.
> >>
> >> A new flag (HCI_LE_SCAN) was created to inform if the controller is
> >> performing LE scan. The HCI_LE_SCAN flag is set/cleared when the
> >> controller starts/stops scanning.
> >>
> >> Signed-off-by: Andre Guedes <[email protected]>
> >> ---
> >> include/net/bluetooth/hci.h | 2 ++
> >> net/bluetooth/hci_event.c | 39 +++++++++++++++++++++++++++++++++
> >> +++---
> >> net/bluetooth/mgmt.c | 5 +++++
> >> 3 files changed, 43 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/
> >> hci.h
> >> index fb40388..c4fdeeb 100644
> >> --- a/include/net/bluetooth/hci.h
> >> +++ b/include/net/bluetooth/hci.h
> >> @@ -86,6 +86,8 @@ enum {
> >> HCI_DEBUG_KEYS,
> >>
> >> HCI_RESET,
> >> +
> >> + HCI_LE_SCAN,
> >> };
> >
> > I am really against adding any new flags here. This is a public API
> > and
> > a horrible one actually.
> >
> > We need to have these states internal and stop adding more flags to
> > this
> > public API.
>
> The HCI_LE_SCAN flag is really device/controller related as well as
> HCI_INQURY flag is. They both have similar meaning and using. Besides,
> the userspace (hciconfig) might be interested in checking this flag to
> know if the controller is carrying out the LE scan (just like it does
> with HCI_INQUIRY flag).
>
> So, may you consider we keep this flag here?
we can keep it for a little bit, but long term, these need to go away.
Especially since they are not available via the mgmt API (and that is
good this way).
Btw. it is not about telling the userspace that we currently run a
discovery procedure, it is more about how the flags are defined and how
bad off an ABI that is.
Regards
Marcel