* [PATCH v7 1/3] Bluetooth: add hci_restart_le_scan
@ 2014-11-20 17:51 Jakub Pawlowski
2014-11-20 17:51 ` [PATCH v7 2/3] Bluetooth: Extract generic start and stop discovery Jakub Pawlowski
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Jakub Pawlowski @ 2014-11-20 17:51 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Jakub Pawlowski
Currently there is no way to restart le scan. It's needed in
preparation for new service scan method. The way it work: it disable,
and then enable le scan on controller. During this restart special flag
is set to make sure we won't remove disable scan work from workqueue.
Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
---
include/net/bluetooth/hci.h | 1 +
include/net/bluetooth/hci_core.h | 2 ++
net/bluetooth/hci_core.c | 42 ++++++++++++++++++++++++++++++++++++++++
net/bluetooth/hci_event.c | 9 ++++++---
4 files changed, 51 insertions(+), 3 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index e56f909..2f0bce2 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -203,6 +203,7 @@ enum {
HCI_FAST_CONNECTABLE,
HCI_BREDR_ENABLED,
HCI_LE_SCAN_INTERRUPTED,
+ HCI_LE_SCAN_RESTARTING,
};
/* A mask for the flags that are supposed to remain when a reset happens
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 396c098..36211f9 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -337,6 +337,7 @@ struct hci_dev {
unsigned long dev_flags;
struct delayed_work le_scan_disable;
+ struct delayed_work le_scan_restart;
__s8 adv_tx_power;
__u8 adv_data[HCI_MAX_AD_LENGTH];
@@ -1403,6 +1404,7 @@ int hci_update_random_address(struct hci_request *req, bool require_privacy,
u8 *own_addr_type);
void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
u8 *bdaddr_type);
+void hci_restart_le_scan(struct hci_dev *hdev, unsigned long delay);
#define SCO_AIRMODE_MASK 0x0003
#define SCO_AIRMODE_CVSD 0x0000
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 5c319a4..858cf54 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2557,6 +2557,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
cancel_delayed_work(&hdev->service_cache);
cancel_delayed_work_sync(&hdev->le_scan_disable);
+ cancel_delayed_work_sync(&hdev->le_scan_restart);
if (test_bit(HCI_MGMT, &hdev->dev_flags))
cancel_delayed_work_sync(&hdev->rpa_expired);
@@ -3851,6 +3852,8 @@ static void le_scan_disable_work(struct work_struct *work)
BT_DBG("%s", hdev->name);
+ cancel_delayed_work_sync(&hdev->le_scan_restart);
+
hci_req_init(&req, hdev);
hci_req_add_le_scan_disable(&req);
@@ -3860,6 +3863,44 @@ static void le_scan_disable_work(struct work_struct *work)
BT_ERR("Disable LE scanning request failed: err %d", err);
}
+void hci_restart_le_scan(struct hci_dev *hdev, unsigned long delay)
+{
+ queue_delayed_work(hdev->workqueue, &hdev->le_scan_restart, delay);
+}
+
+static void le_scan_restart_work_complete(struct hci_dev *hdev, u8 status)
+{
+ clear_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags);
+
+ if (status)
+ BT_ERR("Failed to restart LE scanning: status %d", status);
+}
+
+static void le_scan_restart_work(struct work_struct *work)
+{
+ struct hci_dev *hdev = container_of(work, struct hci_dev,
+ le_scan_restart.work);
+ struct hci_request req;
+ struct hci_cp_le_set_scan_enable cp;
+ int err;
+
+ BT_DBG("%s", hdev->name);
+
+ hci_req_init(&req, hdev);
+
+ hci_req_add_le_scan_disable(&req);
+
+ memset(&cp, 0, sizeof(cp));
+ cp.enable = LE_SCAN_ENABLE;
+ hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
+
+ set_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags);
+
+ err = hci_req_run(&req, le_scan_restart_work_complete);
+ if (err)
+ BT_ERR("Disable LE scanning request failed: err %d", err);
+}
+
static void set_random_addr(struct hci_request *req, bdaddr_t *rpa)
{
struct hci_dev *hdev = req->hdev;
@@ -4037,6 +4078,7 @@ struct hci_dev *hci_alloc_dev(void)
INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
INIT_DELAYED_WORK(&hdev->discov_off, hci_discov_off);
INIT_DELAYED_WORK(&hdev->le_scan_disable, le_scan_disable_work);
+ INIT_DELAYED_WORK(&hdev->le_scan_restart, le_scan_restart_work);
skb_queue_head_init(&hdev->rx_q);
skb_queue_head_init(&hdev->cmd_q);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index bd0a801..ed4d5e1 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1157,10 +1157,13 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
d->last_adv_data_len, NULL, 0);
}
- /* Cancel this timer so that we don't try to disable scanning
- * when it's already disabled.
+ /* If HCI_LE_SCAN_RESTARTING is set, don't cancel this timer,
+ * because we're just restarting scan. Otherwise cancel it so
+ * that we don't try to disable scanning when it's already
+ * disabled.
*/
- cancel_delayed_work(&hdev->le_scan_disable);
+ if (!test_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags))
+ cancel_delayed_work(&hdev->le_scan_disable);
clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v7 2/3] Bluetooth: Extract generic start and stop discovery
2014-11-20 17:51 [PATCH v7 1/3] Bluetooth: add hci_restart_le_scan Jakub Pawlowski
@ 2014-11-20 17:51 ` Jakub Pawlowski
2014-11-20 22:14 ` Arman Uguray
2014-11-20 17:51 ` [PATCH v7 3/3] Bluetooth: start and stop service discovery Jakub Pawlowski
2014-11-20 21:48 ` [PATCH v7 1/3] Bluetooth: add hci_restart_le_scan Arman Uguray
2 siblings, 1 reply; 15+ messages in thread
From: Jakub Pawlowski @ 2014-11-20 17:51 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Jakub Pawlowski
This commit extract generic_start_discovery and generic_stop_discovery
in preparation for start and stop service discovery. The reason behind
that is that both functions will share big part of code, and it would
be much easier to maintain just one generic method.
Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
---
net/bluetooth/mgmt.c | 147 ++++++++++++++++++++++++++++++---------------------
1 file changed, 86 insertions(+), 61 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index cbeef5f..f650d30 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3675,7 +3675,8 @@ done:
return err;
}
-static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status)
+static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status,
+ uint16_t opcode)
{
struct pending_cmd *cmd;
u8 type;
@@ -3683,7 +3684,7 @@ static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status)
hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
- cmd = mgmt_pending_find(MGMT_OP_START_DISCOVERY, hdev);
+ cmd = mgmt_pending_find(opcode, hdev);
if (!cmd)
return -ENOENT;
@@ -3696,7 +3697,8 @@ static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status)
return err;
}
-static void start_discovery_complete(struct hci_dev *hdev, u8 status)
+static void generic_start_discovery_complete(struct hci_dev *hdev, u8 status,
+ uint16_t opcode)
{
unsigned long timeout = 0;
@@ -3704,7 +3706,7 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status)
if (status) {
hci_dev_lock(hdev);
- mgmt_start_discovery_failed(hdev, status);
+ mgmt_start_discovery_failed(hdev, status, opcode);
hci_dev_unlock(hdev);
return;
}
@@ -3735,8 +3737,13 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status)
queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable, timeout);
}
-static int start_discovery(struct sock *sk, struct hci_dev *hdev,
- void *data, u16 len)
+static void start_discovery_complete(struct hci_dev *hdev, u8 status)
+{
+ generic_start_discovery_complete(hdev, status, MGMT_OP_START_DISCOVERY);
+}
+
+static int generic_start_discovery(struct sock *sk, struct hci_dev *hdev,
+ void *data, u16 len, uint16_t opcode)
{
struct mgmt_cp_start_discovery *cp = data;
struct pending_cmd *cmd;
@@ -3746,41 +3753,41 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
struct hci_request req;
/* General inquiry access code (GIAC) */
u8 lap[3] = { 0x33, 0x8b, 0x9e };
- u8 status, own_addr_type;
+ u8 status, own_addr_type, type;
int err;
BT_DBG("%s", hdev->name);
+ type = cp->type;
+
hci_dev_lock(hdev);
if (!hdev_is_powered(hdev)) {
- err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
- MGMT_STATUS_NOT_POWERED,
- &cp->type, sizeof(cp->type));
+ err = cmd_complete(sk, hdev->id, opcode,
+ MGMT_STATUS_NOT_POWERED, &type,
+ sizeof(type));
goto failed;
}
if (test_bit(HCI_PERIODIC_INQ, &hdev->dev_flags)) {
- err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
- MGMT_STATUS_BUSY, &cp->type,
- sizeof(cp->type));
+ err = cmd_complete(sk, hdev->id, opcode,
+ MGMT_STATUS_BUSY, &type, sizeof(type));
goto failed;
}
if (hdev->discovery.state != DISCOVERY_STOPPED) {
- err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
- MGMT_STATUS_BUSY, &cp->type,
- sizeof(cp->type));
+ err = cmd_complete(sk, hdev->id, opcode,
+ MGMT_STATUS_BUSY, &type, sizeof(type));
goto failed;
}
- cmd = mgmt_pending_add(sk, MGMT_OP_START_DISCOVERY, hdev, NULL, 0);
+ cmd = mgmt_pending_add(sk, opcode, hdev, NULL, 0);
if (!cmd) {
err = -ENOMEM;
goto failed;
}
- hdev->discovery.type = cp->type;
+ hdev->discovery.type = type;
hci_req_init(&req, hdev);
@@ -3788,18 +3795,16 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
case DISCOV_TYPE_BREDR:
status = mgmt_bredr_support(hdev);
if (status) {
- err = cmd_complete(sk, hdev->id,
- MGMT_OP_START_DISCOVERY, status,
- &cp->type, sizeof(cp->type));
+ err = cmd_complete(sk, hdev->id, opcode, status, &type,
+ sizeof(type));
mgmt_pending_remove(cmd);
goto failed;
}
if (test_bit(HCI_INQUIRY, &hdev->flags)) {
- err = cmd_complete(sk, hdev->id,
- MGMT_OP_START_DISCOVERY,
- MGMT_STATUS_BUSY, &cp->type,
- sizeof(cp->type));
+ err = cmd_complete(sk, hdev->id, opcode,
+ MGMT_STATUS_BUSY, &type,
+ sizeof(type));
mgmt_pending_remove(cmd);
goto failed;
}
@@ -3816,19 +3821,17 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
case DISCOV_TYPE_INTERLEAVED:
status = mgmt_le_support(hdev);
if (status) {
- err = cmd_complete(sk, hdev->id,
- MGMT_OP_START_DISCOVERY, status,
- &cp->type, sizeof(cp->type));
+ err = cmd_complete(sk, hdev->id, opcode, status, &type,
+ sizeof(type));
mgmt_pending_remove(cmd);
goto failed;
}
if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
!test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
- err = cmd_complete(sk, hdev->id,
- MGMT_OP_START_DISCOVERY,
- MGMT_STATUS_NOT_SUPPORTED,
- &cp->type, sizeof(cp->type));
+ err = cmd_complete(sk, hdev->id, opcode,
+ MGMT_STATUS_NOT_SUPPORTED, &type,
+ sizeof(type));
mgmt_pending_remove(cmd);
goto failed;
}
@@ -3840,11 +3843,9 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
*/
if (hci_conn_hash_lookup_state(hdev, LE_LINK,
BT_CONNECT)) {
- err = cmd_complete(sk, hdev->id,
- MGMT_OP_START_DISCOVERY,
- MGMT_STATUS_REJECTED,
- &cp->type,
- sizeof(cp->type));
+ err = cmd_complete(sk, hdev->id, opcode,
+ MGMT_STATUS_REJECTED, &type,
+ sizeof(type));
mgmt_pending_remove(cmd);
goto failed;
}
@@ -3867,10 +3868,9 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
*/
err = hci_update_random_address(&req, true, &own_addr_type);
if (err < 0) {
- err = cmd_complete(sk, hdev->id,
- MGMT_OP_START_DISCOVERY,
- MGMT_STATUS_FAILED,
- &cp->type, sizeof(cp->type));
+ err = cmd_complete(sk, hdev->id, opcode,
+ MGMT_STATUS_FAILED, &type,
+ sizeof(type));
mgmt_pending_remove(cmd);
goto failed;
}
@@ -3890,14 +3890,15 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
break;
default:
- err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
- MGMT_STATUS_INVALID_PARAMS,
- &cp->type, sizeof(cp->type));
+ err = cmd_complete(sk, hdev->id, opcode,
+ MGMT_STATUS_INVALID_PARAMS, &type,
+ sizeof(type));
mgmt_pending_remove(cmd);
goto failed;
}
err = hci_req_run(&req, start_discovery_complete);
+
if (err < 0)
mgmt_pending_remove(cmd);
else
@@ -3908,12 +3909,20 @@ failed:
return err;
}
-static int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status)
+static int start_discovery(struct sock *sk, struct hci_dev *hdev,
+ void *data, u16 len)
+{
+ return generic_start_discovery(sk, hdev, data, len,
+ MGMT_OP_START_DISCOVERY);
+}
+
+static int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status,
+ uint16_t opcode)
{
struct pending_cmd *cmd;
int err;
- cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, hdev);
+ cmd = mgmt_pending_find(opcode, hdev);
if (!cmd)
return -ENOENT;
@@ -3924,14 +3933,15 @@ static int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status)
return err;
}
-static void stop_discovery_complete(struct hci_dev *hdev, u8 status)
+static void generic_stop_discovery_complete(struct hci_dev *hdev, u8 status,
+ uint16_t opcode)
{
BT_DBG("status %d", status);
hci_dev_lock(hdev);
if (status) {
- mgmt_stop_discovery_failed(hdev, status);
+ mgmt_stop_discovery_failed(hdev, status, opcode);
goto unlock;
}
@@ -3941,33 +3951,40 @@ unlock:
hci_dev_unlock(hdev);
}
-static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
- u16 len)
+static void stop_discovery_complete(struct hci_dev *hdev, u8 status)
+{
+ generic_stop_discovery_complete(hdev, status, MGMT_OP_STOP_DISCOVERY);
+}
+
+static int generic_stop_discovery(struct sock *sk, struct hci_dev *hdev,
+ void *data, u16 len, uint16_t opcode)
{
- struct mgmt_cp_stop_discovery *mgmt_cp = data;
struct pending_cmd *cmd;
struct hci_request req;
+ struct mgmt_cp_stop_discovery *mgmt_cp = data;
int err;
+ u8 type;
BT_DBG("%s", hdev->name);
+ type = mgmt_cp->type;
+
hci_dev_lock(hdev);
if (!hci_discovery_active(hdev)) {
- err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY,
- MGMT_STATUS_REJECTED, &mgmt_cp->type,
- sizeof(mgmt_cp->type));
+ err = cmd_complete(sk, hdev->id, opcode,
+ MGMT_STATUS_REJECTED, &type, sizeof(type));
goto unlock;
}
- if (hdev->discovery.type != mgmt_cp->type) {
- err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY,
- MGMT_STATUS_INVALID_PARAMS, &mgmt_cp->type,
- sizeof(mgmt_cp->type));
+ if (hdev->discovery.type != type) {
+ err = cmd_complete(sk, hdev->id, opcode,
+ MGMT_STATUS_INVALID_PARAMS, &type,
+ sizeof(type));
goto unlock;
}
- cmd = mgmt_pending_add(sk, MGMT_OP_STOP_DISCOVERY, hdev, NULL, 0);
+ cmd = mgmt_pending_add(sk, opcode, hdev, NULL, 0);
if (!cmd) {
err = -ENOMEM;
goto unlock;
@@ -3978,6 +3995,7 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
hci_stop_discovery(&req);
err = hci_req_run(&req, stop_discovery_complete);
+
if (!err) {
hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
goto unlock;
@@ -3987,8 +4005,8 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
/* If no HCI commands were sent we're done */
if (err == -ENODATA) {
- err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY, 0,
- &mgmt_cp->type, sizeof(mgmt_cp->type));
+ err = cmd_complete(sk, hdev->id, opcode, 0,
+ &type, sizeof(type));
hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
}
@@ -3997,6 +4015,13 @@ unlock:
return err;
}
+static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
+ u16 len)
+{
+ return generic_stop_discovery(sk, hdev, data, len,
+ MGMT_OP_STOP_DISCOVERY);
+}
+
static int confirm_name(struct sock *sk, struct hci_dev *hdev, void *data,
u16 len)
{
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v7 3/3] Bluetooth: start and stop service discovery
2014-11-20 17:51 [PATCH v7 1/3] Bluetooth: add hci_restart_le_scan Jakub Pawlowski
2014-11-20 17:51 ` [PATCH v7 2/3] Bluetooth: Extract generic start and stop discovery Jakub Pawlowski
@ 2014-11-20 17:51 ` Jakub Pawlowski
2014-11-20 21:48 ` [PATCH v7 1/3] Bluetooth: add hci_restart_le_scan Arman Uguray
2 siblings, 0 replies; 15+ messages in thread
From: Jakub Pawlowski @ 2014-11-20 17:51 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Jakub Pawlowski
This patch introduces start service discovery and stop service
discovery methods. The reason behind that is to enable users to find
specific services in range by UUID. Whole filtering is done in
mgmt_device_found.
Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
---
include/net/bluetooth/hci.h | 1 +
include/net/bluetooth/hci_core.h | 11 ++
include/net/bluetooth/mgmt.h | 24 ++++
net/bluetooth/hci_core.c | 1 +
net/bluetooth/mgmt.c | 296 +++++++++++++++++++++++++++++++++++++--
5 files changed, 324 insertions(+), 9 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 2f0bce2..97b0893 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -204,6 +204,7 @@ enum {
HCI_BREDR_ENABLED,
HCI_LE_SCAN_INTERRUPTED,
HCI_LE_SCAN_RESTARTING,
+ HCI_SERVICE_FILTER,
};
/* A mask for the flags that are supposed to remain when a reset happens
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 36211f9..39009f2 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -68,6 +68,7 @@ struct discovery_state {
struct list_head all; /* All devices found during inquiry */
struct list_head unknown; /* Name state not known */
struct list_head resolve; /* Name needs to be resolved */
+ struct list_head uuid_filter;
__u32 timestamp;
bdaddr_t last_adv_addr;
u8 last_adv_addr_type;
@@ -146,6 +147,12 @@ struct oob_data {
u8 rand256[16];
};
+struct uuid_filter {
+ struct list_head list;
+ s8 rssi;
+ u8 uuid[16];
+};
+
#define HCI_MAX_SHORT_NAME_LENGTH 10
/* Default LE RPA expiry time, 15 minutes */
@@ -502,6 +509,7 @@ static inline void discovery_init(struct hci_dev *hdev)
INIT_LIST_HEAD(&hdev->discovery.all);
INIT_LIST_HEAD(&hdev->discovery.unknown);
INIT_LIST_HEAD(&hdev->discovery.resolve);
+ INIT_LIST_HEAD(&hdev->discovery.uuid_filter);
}
bool hci_discovery_active(struct hci_dev *hdev);
@@ -1328,6 +1336,8 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event);
#define DISCOV_INTERLEAVED_INQUIRY_LEN 0x04
#define DISCOV_BREDR_INQUIRY_LEN 0x08
+#define DISCOV_LE_RESTART_DELAY msecs_to_jiffies(300)
+
int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
int mgmt_new_settings(struct hci_dev *hdev);
void mgmt_index_added(struct hci_dev *hdev);
@@ -1394,6 +1404,7 @@ void mgmt_new_conn_param(struct hci_dev *hdev, bdaddr_t *bdaddr,
u16 max_interval, u16 latency, u16 timeout);
void mgmt_reenable_advertising(struct hci_dev *hdev);
void mgmt_smp_complete(struct hci_conn *conn, bool complete);
+void mgmt_clean_up_service_discovery(struct hci_dev *hdev);
u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
u16 to_multiplier);
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index b391fd6..202caf7 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -495,6 +495,30 @@ struct mgmt_cp_set_public_address {
} __packed;
#define MGMT_SET_PUBLIC_ADDRESS_SIZE 6
+#define MGMT_OP_START_SERVICE_DISCOVERY 0x003A
+#define MGMT_START_SERVICE_DISCOVERY_SIZE 1
+
+#define MGMT_RANGE_NONE 0x00
+#define MGMT_RANGE_RSSI 0x01
+#define MGMT_RANGE_PATHLOSS 0x02
+
+struct mgmt_uuid_filter {
+ __s8 rssi;
+ __u8 uuid[16];
+} __packed;
+
+struct mgmt_cp_start_service_discovery {
+ __u8 type;
+ __le16 filter_count;
+ struct mgmt_uuid_filter filter[0];
+} __packed;
+
+#define MGMT_OP_STOP_SERVICE_DISCOVERY 0x003B
+struct mgmt_cp_stop_service_discovery {
+ __u8 type;
+} __packed;
+#define MGMT_STOP_SERVICE_DISCOVERY_SIZE 1
+
#define MGMT_EV_CMD_COMPLETE 0x0001
struct mgmt_ev_cmd_complete {
__le16 opcode;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 858cf54..fab6755 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3853,6 +3853,7 @@ static void le_scan_disable_work(struct work_struct *work)
BT_DBG("%s", hdev->name);
cancel_delayed_work_sync(&hdev->le_scan_restart);
+ mgmt_clean_up_service_discovery(hdev);
hci_req_init(&req, hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index f650d30..ed67060 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -93,6 +93,8 @@ static const u16 mgmt_commands[] = {
MGMT_OP_READ_CONFIG_INFO,
MGMT_OP_SET_EXTERNAL_CONFIG,
MGMT_OP_SET_PUBLIC_ADDRESS,
+ MGMT_OP_START_SERVICE_DISCOVERY,
+ MGMT_OP_STOP_SERVICE_DISCOVERY,
};
static const u16 mgmt_events[] = {
@@ -1258,6 +1260,23 @@ static void clean_up_hci_complete(struct hci_dev *hdev, u8 status)
}
}
+void mgmt_clean_up_service_discovery(struct hci_dev *hdev)
+{
+ struct uuid_filter *filter;
+ struct uuid_filter *tmp;
+
+ if (!test_and_clear_bit(HCI_SERVICE_FILTER, &hdev->dev_flags))
+ return;
+
+ cancel_delayed_work_sync(&hdev->le_scan_restart);
+
+ list_for_each_entry_safe(filter, tmp, &hdev->discovery.uuid_filter,
+ list) {
+ __list_del_entry(&filter->list);
+ kfree(filter);
+ }
+}
+
static bool hci_stop_discovery(struct hci_request *req)
{
struct hci_dev *hdev = req->hdev;
@@ -1270,6 +1289,7 @@ static bool hci_stop_discovery(struct hci_request *req)
hci_req_add(req, HCI_OP_INQUIRY_CANCEL, 0, NULL);
} else {
cancel_delayed_work(&hdev->le_scan_disable);
+ mgmt_clean_up_service_discovery(hdev);
hci_req_add_le_scan_disable(req);
}
@@ -3742,23 +3762,75 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status)
generic_start_discovery_complete(hdev, status, MGMT_OP_START_DISCOVERY);
}
+static void start_service_discovery_complete(struct hci_dev *hdev, u8 status)
+{
+ generic_start_discovery_complete(hdev, status,
+ MGMT_OP_START_SERVICE_DISCOVERY);
+}
+
+static int init_service_discovery(struct hci_dev *hdev,
+ struct mgmt_uuid_filter *filters,
+ u16 filter_cnt)
+{
+ int i;
+
+ for (i = 0; i < filter_cnt; i++) {
+ struct mgmt_uuid_filter *key = &filters[i];
+ struct uuid_filter *tmp;
+
+ tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ memcpy(tmp->uuid, key->uuid, 16);
+ tmp->rssi = key->rssi;
+ INIT_LIST_HEAD(&tmp->list);
+
+ list_add(&tmp->list, &hdev->discovery.uuid_filter);
+ }
+ set_bit(HCI_SERVICE_FILTER, &hdev->dev_flags);
+ return 0;
+}
+
static int generic_start_discovery(struct sock *sk, struct hci_dev *hdev,
void *data, u16 len, uint16_t opcode)
{
- struct mgmt_cp_start_discovery *cp = data;
struct pending_cmd *cmd;
struct hci_cp_le_set_scan_param param_cp;
struct hci_cp_le_set_scan_enable enable_cp;
struct hci_cp_inquiry inq_cp;
struct hci_request req;
+ struct mgmt_uuid_filter *serv_filters = NULL;
/* General inquiry access code (GIAC) */
u8 lap[3] = { 0x33, 0x8b, 0x9e };
u8 status, own_addr_type, type;
int err;
+ u16 serv_filter_cnt = 0;
BT_DBG("%s", hdev->name);
- type = cp->type;
+ if (opcode == MGMT_OP_START_SERVICE_DISCOVERY) {
+ struct mgmt_cp_start_service_discovery *cp = data;
+ u16 expected_len, filter_count;
+
+ type = cp->type;
+ filter_count = __le16_to_cpu(cp->filter_count);
+ expected_len = sizeof(*cp) +
+ filter_count * sizeof(struct mgmt_uuid_filter);
+
+ if (expected_len != len) {
+ return cmd_complete(sk, hdev->id, opcode,
+ MGMT_STATUS_INVALID_PARAMS, &type,
+ sizeof(type));
+ }
+
+ serv_filters = cp->filter;
+ serv_filter_cnt = filter_count;
+ } else {
+ struct mgmt_cp_start_discovery *cp = data;
+
+ type = cp->type;
+ }
hci_dev_lock(hdev);
@@ -3897,7 +3969,17 @@ static int generic_start_discovery(struct sock *sk, struct hci_dev *hdev,
goto failed;
}
- err = hci_req_run(&req, start_discovery_complete);
+ if (serv_filters != NULL) {
+ err = init_service_discovery(hdev, serv_filters,
+ serv_filter_cnt);
+ if (err)
+ goto failed;
+ }
+
+ if (opcode == MGMT_OP_START_SERVICE_DISCOVERY)
+ err = hci_req_run(&req, start_service_discovery_complete);
+ else
+ err = hci_req_run(&req, start_discovery_complete);
if (err < 0)
mgmt_pending_remove(cmd);
@@ -3956,18 +4038,31 @@ static void stop_discovery_complete(struct hci_dev *hdev, u8 status)
generic_stop_discovery_complete(hdev, status, MGMT_OP_STOP_DISCOVERY);
}
+static void stop_service_discovery_complete(struct hci_dev *hdev, u8 status)
+{
+ generic_stop_discovery_complete(hdev, status,
+ MGMT_OP_STOP_SERVICE_DISCOVERY);
+}
+
static int generic_stop_discovery(struct sock *sk, struct hci_dev *hdev,
void *data, u16 len, uint16_t opcode)
{
struct pending_cmd *cmd;
struct hci_request req;
- struct mgmt_cp_stop_discovery *mgmt_cp = data;
int err;
u8 type;
BT_DBG("%s", hdev->name);
- type = mgmt_cp->type;
+ if (opcode == MGMT_OP_STOP_SERVICE_DISCOVERY) {
+ struct mgmt_cp_stop_service_discovery *mgmt_cp = data;
+
+ type = mgmt_cp->type;
+ } else {
+ struct mgmt_cp_stop_discovery *mgmt_cp = data;
+
+ type = mgmt_cp->type;
+ }
hci_dev_lock(hdev);
@@ -3994,7 +4089,10 @@ static int generic_stop_discovery(struct sock *sk, struct hci_dev *hdev,
hci_stop_discovery(&req);
- err = hci_req_run(&req, stop_discovery_complete);
+ if (opcode == MGMT_OP_STOP_SERVICE_DISCOVERY)
+ err = hci_req_run(&req, stop_service_discovery_complete);
+ else
+ err = hci_req_run(&req, stop_discovery_complete);
if (!err) {
hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
@@ -5707,6 +5805,20 @@ unlock:
return err;
}
+static int start_service_discovery(struct sock *sk, struct hci_dev *hdev,
+ void *data, u16 len)
+{
+ return generic_start_discovery(sk, hdev, data, len,
+ MGMT_OP_START_SERVICE_DISCOVERY);
+}
+
+static int stop_service_discovery(struct sock *sk, struct hci_dev *hdev,
+ void *data, u16 len)
+{
+ return generic_stop_discovery(sk, hdev, data, len,
+ MGMT_OP_STOP_SERVICE_DISCOVERY);
+}
+
static const struct mgmt_handler {
int (*func) (struct sock *sk, struct hci_dev *hdev, void *data,
u16 data_len);
@@ -5771,6 +5883,8 @@ static const struct mgmt_handler {
{ read_config_info, false, MGMT_READ_CONFIG_INFO_SIZE },
{ set_external_config, false, MGMT_SET_EXTERNAL_CONFIG_SIZE },
{ set_public_address, false, MGMT_SET_PUBLIC_ADDRESS_SIZE },
+ { start_service_discovery, true, MGMT_START_SERVICE_DISCOVERY_SIZE },
+ { stop_service_discovery, false, MGMT_STOP_SERVICE_DISCOVERY_SIZE },
};
int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
@@ -6837,6 +6951,135 @@ void mgmt_read_local_oob_data_complete(struct hci_dev *hdev, u8 *hash192,
mgmt_pending_remove(cmd);
}
+struct parsed_uuid {
+ struct list_head list;
+ u8 uuid[16];
+};
+
+/* this is reversed hex representation of bluetooth base uuid. We need it for
+ * service uuid parsing in eir.
+ */
+static const u8 reverse_base_uuid[] = {
+ 0xfb, 0x34, 0x9b, 0x5f, 0x80, 0x00, 0x00, 0x80,
+ 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+};
+
+static int add_uuid_to_list(struct list_head *uuids, u8 *uuid)
+{
+ struct parsed_uuid *tmp_uuid;
+
+ tmp_uuid = kmalloc(sizeof(*tmp_uuid), GFP_KERNEL);
+ if (tmp_uuid == NULL)
+ return -ENOMEM;
+
+ memcpy(tmp_uuid->uuid, uuid, 16);
+ INIT_LIST_HEAD(&tmp_uuid->list);
+ list_add(&tmp_uuid->list, uuids);
+ return 0;
+}
+
+static void free_uuids_list(struct list_head *uuids)
+{
+ struct parsed_uuid *uuid, *tmp;
+
+ list_for_each_entry_safe(uuid, tmp, uuids, list) {
+ __list_del_entry(&uuid->list);
+ kfree(uuid);
+ }
+}
+
+static int eir_parse(u8 *eir, u8 eir_len, struct list_head *uuids)
+{
+ size_t offset;
+ u8 uuid[16];
+ int i, ret;
+
+ offset = 0;
+ while (offset < eir_len) {
+ uint8_t field_len = eir[0];
+
+ /* Check for the end of EIR */
+ if (field_len == 0)
+ break;
+
+ if (offset + field_len > eir_len)
+ return -EINVAL;
+
+ switch (eir[1]) {
+ case EIR_UUID16_ALL:
+ case EIR_UUID16_SOME:
+ for (i = 0; i + 3 <= field_len; i += 2) {
+ memcpy(uuid, reverse_base_uuid, 16);
+ uuid[13] = eir[i + 3];
+ uuid[12] = eir[i + 2];
+ ret = add_uuid_to_list(uuids, uuid);
+ if (ret)
+ return ret;
+ }
+ break;
+ case EIR_UUID32_ALL:
+ case EIR_UUID32_SOME:
+ for (i = 0; i + 5 <= field_len; i += 4) {
+ memcpy(uuid, reverse_base_uuid, 16);
+ uuid[15] = eir[i + 5];
+ uuid[14] = eir[i + 4];
+ uuid[13] = eir[i + 3];
+ uuid[12] = eir[i + 2];
+ ret = add_uuid_to_list(uuids, uuid);
+ if (ret)
+ return ret;
+ }
+ break;
+ case EIR_UUID128_ALL:
+ case EIR_UUID128_SOME:
+ for (i = 0; i + 17 <= field_len; i += 16) {
+ memcpy(uuid, eir + i + 2, 16);
+ ret = add_uuid_to_list(uuids, uuid);
+ if (ret)
+ return ret;
+ }
+ break;
+ }
+
+ offset += field_len + 1;
+ eir += field_len + 1;
+ }
+ return 0;
+}
+
+enum {
+ NO_MATCH,
+ SERVICE_MATCH,
+ FULL_MATCH
+};
+
+static u8 find_match(struct uuid_filter *filter, u8 uuid[16], s8 rssi)
+{
+ if (memcmp(filter->uuid, uuid, 16) != 0)
+ return NO_MATCH;
+ if (rssi >= filter->rssi)
+ return FULL_MATCH;
+
+ return SERVICE_MATCH;
+}
+
+static u8 find_matches(struct hci_dev *hdev, s8 rssi, struct list_head *uuids)
+{
+ struct uuid_filter *filter;
+ struct parsed_uuid *uuidptr, *tmp_uuid;
+ int match_type = NO_MATCH, tmp;
+
+ list_for_each_entry_safe(uuidptr, tmp_uuid, uuids, list) {
+ list_for_each_entry(filter, &hdev->discovery.uuid_filter,
+ list) {
+ tmp = find_match(filter, uuidptr->uuid, rssi);
+ if (tmp > match_type)
+ match_type = tmp;
+ }
+ }
+ return match_type;
+}
+
void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
u8 addr_type, u8 *dev_class, s8 rssi, u32 flags,
u8 *eir, u16 eir_len, u8 *scan_rsp, u8 scan_rsp_len)
@@ -6844,6 +7087,9 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
char buf[512];
struct mgmt_ev_device_found *ev = (void *) buf;
size_t ev_size;
+ LIST_HEAD(uuids);
+ int err = 0;
+ u8 match_type;
/* Don't send events for a non-kernel initiated discovery. With
* LE one exception is if we have pend_le_reports > 0 in which
@@ -6882,7 +7128,31 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
ev_size = sizeof(*ev) + eir_len + scan_rsp_len;
- mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
+ if (!test_bit(HCI_SERVICE_FILTER, &hdev->dev_flags)) {
+ mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
+ return;
+ }
+
+ err = eir_parse(eir, eir_len, &uuids);
+ if (err) {
+ free_uuids_list(&uuids);
+ return;
+ }
+
+ match_type = find_matches(hdev, rssi, &uuids);
+ free_uuids_list(&uuids);
+
+ if (match_type == NO_MATCH)
+ return;
+
+ if (test_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks)) {
+ queue_delayed_work(hdev->workqueue,
+ &hdev->le_scan_restart,
+ DISCOV_LE_RESTART_DELAY);
+ }
+ if (match_type == FULL_MATCH)
+ mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev,
+ ev_size, NULL);
}
void mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
@@ -6915,10 +7185,18 @@ void mgmt_discovering(struct hci_dev *hdev, u8 discovering)
BT_DBG("%s discovering %u", hdev->name, discovering);
- if (discovering)
+ if (discovering) {
cmd = mgmt_pending_find(MGMT_OP_START_DISCOVERY, hdev);
- else
+ if (cmd == NULL)
+ cmd = mgmt_pending_find(MGMT_OP_START_SERVICE_DISCOVERY,
+ hdev);
+
+ } else {
cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, hdev);
+ if (cmd == NULL)
+ cmd = mgmt_pending_find(MGMT_OP_STOP_SERVICE_DISCOVERY,
+ hdev);
+ }
if (cmd != NULL) {
u8 type = hdev->discovery.type;
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v7 1/3] Bluetooth: add hci_restart_le_scan
2014-11-20 17:51 [PATCH v7 1/3] Bluetooth: add hci_restart_le_scan Jakub Pawlowski
2014-11-20 17:51 ` [PATCH v7 2/3] Bluetooth: Extract generic start and stop discovery Jakub Pawlowski
2014-11-20 17:51 ` [PATCH v7 3/3] Bluetooth: start and stop service discovery Jakub Pawlowski
@ 2014-11-20 21:48 ` Arman Uguray
2014-11-20 22:24 ` Jakub Pawlowski
2 siblings, 1 reply; 15+ messages in thread
From: Arman Uguray @ 2014-11-20 21:48 UTC (permalink / raw)
To: Jakub Pawlowski; +Cc: BlueZ development
Hi Jakub,
> On Thu, Nov 20, 2014 at 9:51 AM, Jakub Pawlowski <jpawlowski@google.com> wrote:
> Currently there is no way to restart le scan. It's needed in
> preparation for new service scan method. The way it work: it disable,
> and then enable le scan on controller. During this restart special flag
> is set to make sure we won't remove disable scan work from workqueue.
>
> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
> ---
> include/net/bluetooth/hci.h | 1 +
> include/net/bluetooth/hci_core.h | 2 ++
> net/bluetooth/hci_core.c | 42 ++++++++++++++++++++++++++++++++++++++++
> net/bluetooth/hci_event.c | 9 ++++++---
> 4 files changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index e56f909..2f0bce2 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -203,6 +203,7 @@ enum {
> HCI_FAST_CONNECTABLE,
> HCI_BREDR_ENABLED,
> HCI_LE_SCAN_INTERRUPTED,
> + HCI_LE_SCAN_RESTARTING,
> };
>
> /* A mask for the flags that are supposed to remain when a reset happens
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 396c098..36211f9 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -337,6 +337,7 @@ struct hci_dev {
> unsigned long dev_flags;
>
> struct delayed_work le_scan_disable;
> + struct delayed_work le_scan_restart;
>
> __s8 adv_tx_power;
> __u8 adv_data[HCI_MAX_AD_LENGTH];
> @@ -1403,6 +1404,7 @@ int hci_update_random_address(struct hci_request *req, bool require_privacy,
> u8 *own_addr_type);
> void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
> u8 *bdaddr_type);
> +void hci_restart_le_scan(struct hci_dev *hdev, unsigned long delay);
>
> #define SCO_AIRMODE_MASK 0x0003
> #define SCO_AIRMODE_CVSD 0x0000
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 5c319a4..858cf54 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2557,6 +2557,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> cancel_delayed_work(&hdev->service_cache);
>
> cancel_delayed_work_sync(&hdev->le_scan_disable);
> + cancel_delayed_work_sync(&hdev->le_scan_restart);
>
> if (test_bit(HCI_MGMT, &hdev->dev_flags))
> cancel_delayed_work_sync(&hdev->rpa_expired);
> @@ -3851,6 +3852,8 @@ static void le_scan_disable_work(struct work_struct *work)
>
> BT_DBG("%s", hdev->name);
>
> + cancel_delayed_work_sync(&hdev->le_scan_restart);
> +
> hci_req_init(&req, hdev);
>
> hci_req_add_le_scan_disable(&req);
> @@ -3860,6 +3863,44 @@ static void le_scan_disable_work(struct work_struct *work)
> BT_ERR("Disable LE scanning request failed: err %d", err);
> }
>
> +void hci_restart_le_scan(struct hci_dev *hdev, unsigned long delay)
> +{
> + queue_delayed_work(hdev->workqueue, &hdev->le_scan_restart, delay);
> +}
Since you're exposing this as a public API, does it make sense to
check here if a delayed work has already been scheduled? I.e. is it OK
if this function gets called several times in succession with
different delay values?
> +
> +static void le_scan_restart_work_complete(struct hci_dev *hdev, u8 status)
> +{
> + clear_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags);
> +
> + if (status)
> + BT_ERR("Failed to restart LE scanning: status %d", status);
> +}
> +
> +static void le_scan_restart_work(struct work_struct *work)
> +{
> + struct hci_dev *hdev = container_of(work, struct hci_dev,
> + le_scan_restart.work);
> + struct hci_request req;
> + struct hci_cp_le_set_scan_enable cp;
> + int err;
> +
> + BT_DBG("%s", hdev->name);
> +
> + hci_req_init(&req, hdev);
> +
> + hci_req_add_le_scan_disable(&req);
> +
> + memset(&cp, 0, sizeof(cp));
> + cp.enable = LE_SCAN_ENABLE;
> + hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
> +
> + set_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags);
> +
> + err = hci_req_run(&req, le_scan_restart_work_complete);
> + if (err)
> + BT_ERR("Disable LE scanning request failed: err %d", err);
> +}
> +
> static void set_random_addr(struct hci_request *req, bdaddr_t *rpa)
> {
> struct hci_dev *hdev = req->hdev;
> @@ -4037,6 +4078,7 @@ struct hci_dev *hci_alloc_dev(void)
> INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
> INIT_DELAYED_WORK(&hdev->discov_off, hci_discov_off);
> INIT_DELAYED_WORK(&hdev->le_scan_disable, le_scan_disable_work);
> + INIT_DELAYED_WORK(&hdev->le_scan_restart, le_scan_restart_work);
>
> skb_queue_head_init(&hdev->rx_q);
> skb_queue_head_init(&hdev->cmd_q);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index bd0a801..ed4d5e1 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1157,10 +1157,13 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
> d->last_adv_data_len, NULL, 0);
> }
>
> - /* Cancel this timer so that we don't try to disable scanning
> - * when it's already disabled.
> + /* If HCI_LE_SCAN_RESTARTING is set, don't cancel this timer,
> + * because we're just restarting scan. Otherwise cancel it so
> + * that we don't try to disable scanning when it's already
> + * disabled.
> */
> - cancel_delayed_work(&hdev->le_scan_disable);
> + if (!test_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags))
> + cancel_delayed_work(&hdev->le_scan_disable);
>
> clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
>
> --
> 2.1.0.rc2.206.gedb03e5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks,
Arman
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 2/3] Bluetooth: Extract generic start and stop discovery
2014-11-20 17:51 ` [PATCH v7 2/3] Bluetooth: Extract generic start and stop discovery Jakub Pawlowski
@ 2014-11-20 22:14 ` Arman Uguray
2014-11-20 23:14 ` Jakub Pawlowski
0 siblings, 1 reply; 15+ messages in thread
From: Arman Uguray @ 2014-11-20 22:14 UTC (permalink / raw)
To: Jakub Pawlowski; +Cc: BlueZ development
Hi Jakub,
> On Thu, Nov 20, 2014 at 9:51 AM, Jakub Pawlowski <jpawlowski@google.com> wrote:
> This commit extract generic_start_discovery and generic_stop_discovery
> in preparation for start and stop service discovery. The reason behind
> that is that both functions will share big part of code, and it would
> be much easier to maintain just one generic method.
>
> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
> ---
> net/bluetooth/mgmt.c | 147 ++++++++++++++++++++++++++++++---------------------
> 1 file changed, 86 insertions(+), 61 deletions(-)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index cbeef5f..f650d30 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -3675,7 +3675,8 @@ done:
> return err;
> }
>
> -static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status)
> +static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status,
> + uint16_t opcode)
> {
> struct pending_cmd *cmd;
> u8 type;
> @@ -3683,7 +3684,7 @@ static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status)
>
> hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>
> - cmd = mgmt_pending_find(MGMT_OP_START_DISCOVERY, hdev);
> + cmd = mgmt_pending_find(opcode, hdev);
> if (!cmd)
> return -ENOENT;
>
> @@ -3696,7 +3697,8 @@ static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status)
> return err;
> }
>
> -static void start_discovery_complete(struct hci_dev *hdev, u8 status)
> +static void generic_start_discovery_complete(struct hci_dev *hdev, u8 status,
> + uint16_t opcode)
I'm not that big on calling these generic functions "generic_*". Let's
try to come up with something better.
> {
> unsigned long timeout = 0;
>
> @@ -3704,7 +3706,7 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status)
>
> if (status) {
> hci_dev_lock(hdev);
> - mgmt_start_discovery_failed(hdev, status);
> + mgmt_start_discovery_failed(hdev, status, opcode);
> hci_dev_unlock(hdev);
> return;
> }
> @@ -3735,8 +3737,13 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status)
> queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable, timeout);
> }
>
> -static int start_discovery(struct sock *sk, struct hci_dev *hdev,
> - void *data, u16 len)
> +static void start_discovery_complete(struct hci_dev *hdev, u8 status)
> +{
> + generic_start_discovery_complete(hdev, status, MGMT_OP_START_DISCOVERY);
> +}
> +
> +static int generic_start_discovery(struct sock *sk, struct hci_dev *hdev,
> + void *data, u16 len, uint16_t opcode)
> {
> struct mgmt_cp_start_discovery *cp = data;
> struct pending_cmd *cmd;
> @@ -3746,41 +3753,41 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
> struct hci_request req;
> /* General inquiry access code (GIAC) */
> u8 lap[3] = { 0x33, 0x8b, 0x9e };
> - u8 status, own_addr_type;
> + u8 status, own_addr_type, type;
> int err;
>
> BT_DBG("%s", hdev->name);
>
> + type = cp->type;
> +
I don't see the point of this local variable & assignment. Why not
just leave it as it is, "cp->type" isn't that wordy.
> hci_dev_lock(hdev);
>
> if (!hdev_is_powered(hdev)) {
> - err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
> - MGMT_STATUS_NOT_POWERED,
> - &cp->type, sizeof(cp->type));
> + err = cmd_complete(sk, hdev->id, opcode,
> + MGMT_STATUS_NOT_POWERED, &type,
> + sizeof(type));
> goto failed;
> }
>
> if (test_bit(HCI_PERIODIC_INQ, &hdev->dev_flags)) {
> - err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
> - MGMT_STATUS_BUSY, &cp->type,
> - sizeof(cp->type));
> + err = cmd_complete(sk, hdev->id, opcode,
> + MGMT_STATUS_BUSY, &type, sizeof(type));
> goto failed;
> }
>
> if (hdev->discovery.state != DISCOVERY_STOPPED) {
> - err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
> - MGMT_STATUS_BUSY, &cp->type,
> - sizeof(cp->type));
> + err = cmd_complete(sk, hdev->id, opcode,
> + MGMT_STATUS_BUSY, &type, sizeof(type));
> goto failed;
> }
>
> - cmd = mgmt_pending_add(sk, MGMT_OP_START_DISCOVERY, hdev, NULL, 0);
> + cmd = mgmt_pending_add(sk, opcode, hdev, NULL, 0);
> if (!cmd) {
> err = -ENOMEM;
> goto failed;
> }
>
> - hdev->discovery.type = cp->type;
> + hdev->discovery.type = type;
>
> hci_req_init(&req, hdev);
>
> @@ -3788,18 +3795,16 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
> case DISCOV_TYPE_BREDR:
> status = mgmt_bredr_support(hdev);
> if (status) {
> - err = cmd_complete(sk, hdev->id,
> - MGMT_OP_START_DISCOVERY, status,
> - &cp->type, sizeof(cp->type));
> + err = cmd_complete(sk, hdev->id, opcode, status, &type,
> + sizeof(type));
> mgmt_pending_remove(cmd);
> goto failed;
> }
>
> if (test_bit(HCI_INQUIRY, &hdev->flags)) {
> - err = cmd_complete(sk, hdev->id,
> - MGMT_OP_START_DISCOVERY,
> - MGMT_STATUS_BUSY, &cp->type,
> - sizeof(cp->type));
> + err = cmd_complete(sk, hdev->id, opcode,
> + MGMT_STATUS_BUSY, &type,
> + sizeof(type));
> mgmt_pending_remove(cmd);
> goto failed;
> }
> @@ -3816,19 +3821,17 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
> case DISCOV_TYPE_INTERLEAVED:
> status = mgmt_le_support(hdev);
> if (status) {
> - err = cmd_complete(sk, hdev->id,
> - MGMT_OP_START_DISCOVERY, status,
> - &cp->type, sizeof(cp->type));
> + err = cmd_complete(sk, hdev->id, opcode, status, &type,
> + sizeof(type));
> mgmt_pending_remove(cmd);
> goto failed;
> }
>
> if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
> !test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
> - err = cmd_complete(sk, hdev->id,
> - MGMT_OP_START_DISCOVERY,
> - MGMT_STATUS_NOT_SUPPORTED,
> - &cp->type, sizeof(cp->type));
> + err = cmd_complete(sk, hdev->id, opcode,
> + MGMT_STATUS_NOT_SUPPORTED, &type,
> + sizeof(type));
> mgmt_pending_remove(cmd);
> goto failed;
> }
> @@ -3840,11 +3843,9 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
> */
> if (hci_conn_hash_lookup_state(hdev, LE_LINK,
> BT_CONNECT)) {
> - err = cmd_complete(sk, hdev->id,
> - MGMT_OP_START_DISCOVERY,
> - MGMT_STATUS_REJECTED,
> - &cp->type,
> - sizeof(cp->type));
> + err = cmd_complete(sk, hdev->id, opcode,
> + MGMT_STATUS_REJECTED, &type,
> + sizeof(type));
> mgmt_pending_remove(cmd);
> goto failed;
> }
> @@ -3867,10 +3868,9 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
> */
> err = hci_update_random_address(&req, true, &own_addr_type);
> if (err < 0) {
> - err = cmd_complete(sk, hdev->id,
> - MGMT_OP_START_DISCOVERY,
> - MGMT_STATUS_FAILED,
> - &cp->type, sizeof(cp->type));
> + err = cmd_complete(sk, hdev->id, opcode,
> + MGMT_STATUS_FAILED, &type,
> + sizeof(type));
> mgmt_pending_remove(cmd);
> goto failed;
> }
> @@ -3890,14 +3890,15 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
> break;
>
> default:
> - err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
> - MGMT_STATUS_INVALID_PARAMS,
> - &cp->type, sizeof(cp->type));
> + err = cmd_complete(sk, hdev->id, opcode,
> + MGMT_STATUS_INVALID_PARAMS, &type,
> + sizeof(type));
> mgmt_pending_remove(cmd);
> goto failed;
> }
>
> err = hci_req_run(&req, start_discovery_complete);
> +
> if (err < 0)
> mgmt_pending_remove(cmd);
> else
> @@ -3908,12 +3909,20 @@ failed:
> return err;
> }
>
> -static int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status)
> +static int start_discovery(struct sock *sk, struct hci_dev *hdev,
> + void *data, u16 len)
> +{
> + return generic_start_discovery(sk, hdev, data, len,
> + MGMT_OP_START_DISCOVERY);
> +}
> +
> +static int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status,
> + uint16_t opcode)
> {
> struct pending_cmd *cmd;
> int err;
>
> - cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, hdev);
> + cmd = mgmt_pending_find(opcode, hdev);
> if (!cmd)
> return -ENOENT;
>
> @@ -3924,14 +3933,15 @@ static int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status)
> return err;
> }
>
> -static void stop_discovery_complete(struct hci_dev *hdev, u8 status)
> +static void generic_stop_discovery_complete(struct hci_dev *hdev, u8 status,
> + uint16_t opcode)
> {
> BT_DBG("status %d", status);
>
> hci_dev_lock(hdev);
>
> if (status) {
> - mgmt_stop_discovery_failed(hdev, status);
> + mgmt_stop_discovery_failed(hdev, status, opcode);
> goto unlock;
> }
>
> @@ -3941,33 +3951,40 @@ unlock:
> hci_dev_unlock(hdev);
> }
>
> -static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
> - u16 len)
> +static void stop_discovery_complete(struct hci_dev *hdev, u8 status)
> +{
> + generic_stop_discovery_complete(hdev, status, MGMT_OP_STOP_DISCOVERY);
> +}
> +
> +static int generic_stop_discovery(struct sock *sk, struct hci_dev *hdev,
> + void *data, u16 len, uint16_t opcode)
> {
> - struct mgmt_cp_stop_discovery *mgmt_cp = data;
> struct pending_cmd *cmd;
> struct hci_request req;
> + struct mgmt_cp_stop_discovery *mgmt_cp = data;
Why move this?
> int err;
> + u8 type;
>
Again, there's no need for this variable.
> BT_DBG("%s", hdev->name);
>
> + type = mgmt_cp->type;
> +
> hci_dev_lock(hdev);
>
> if (!hci_discovery_active(hdev)) {
> - err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY,
> - MGMT_STATUS_REJECTED, &mgmt_cp->type,
> - sizeof(mgmt_cp->type));
> + err = cmd_complete(sk, hdev->id, opcode,
> + MGMT_STATUS_REJECTED, &type, sizeof(type));
> goto unlock;
> }
>
> - if (hdev->discovery.type != mgmt_cp->type) {
> - err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY,
> - MGMT_STATUS_INVALID_PARAMS, &mgmt_cp->type,
> - sizeof(mgmt_cp->type));
> + if (hdev->discovery.type != type) {
> + err = cmd_complete(sk, hdev->id, opcode,
> + MGMT_STATUS_INVALID_PARAMS, &type,
> + sizeof(type));
> goto unlock;
> }
>
> - cmd = mgmt_pending_add(sk, MGMT_OP_STOP_DISCOVERY, hdev, NULL, 0);
> + cmd = mgmt_pending_add(sk, opcode, hdev, NULL, 0);
> if (!cmd) {
> err = -ENOMEM;
> goto unlock;
> @@ -3978,6 +3995,7 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
> hci_stop_discovery(&req);
>
> err = hci_req_run(&req, stop_discovery_complete);
> +
> if (!err) {
> hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
> goto unlock;
> @@ -3987,8 +4005,8 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>
> /* If no HCI commands were sent we're done */
> if (err == -ENODATA) {
> - err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY, 0,
> - &mgmt_cp->type, sizeof(mgmt_cp->type));
> + err = cmd_complete(sk, hdev->id, opcode, 0,
> + &type, sizeof(type));
> hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> }
>
> @@ -3997,6 +4015,13 @@ unlock:
> return err;
> }
>
> +static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
> + u16 len)
> +{
> + return generic_stop_discovery(sk, hdev, data, len,
> + MGMT_OP_STOP_DISCOVERY);
> +}
> +
> static int confirm_name(struct sock *sk, struct hci_dev *hdev, void *data,
> u16 len)
> {
> --
> 2.1.0.rc2.206.gedb03e5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Overall I think this patch looks good, except for a few nits that I
commented on above.
Thanks,
Arman
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 1/3] Bluetooth: add hci_restart_le_scan
2014-11-20 21:48 ` [PATCH v7 1/3] Bluetooth: add hci_restart_le_scan Arman Uguray
@ 2014-11-20 22:24 ` Jakub Pawlowski
2014-11-20 23:37 ` Arman Uguray
0 siblings, 1 reply; 15+ messages in thread
From: Jakub Pawlowski @ 2014-11-20 22:24 UTC (permalink / raw)
To: Arman Uguray; +Cc: BlueZ development
On Thu, Nov 20, 2014 at 1:48 PM, Arman Uguray <armansito@chromium.org> wrote:
> Hi Jakub,
>
>> On Thu, Nov 20, 2014 at 9:51 AM, Jakub Pawlowski <jpawlowski@google.com> wrote:
>> Currently there is no way to restart le scan. It's needed in
>> preparation for new service scan method. The way it work: it disable,
>> and then enable le scan on controller. During this restart special flag
>> is set to make sure we won't remove disable scan work from workqueue.
>>
>> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
>> ---
>> include/net/bluetooth/hci.h | 1 +
>> include/net/bluetooth/hci_core.h | 2 ++
>> net/bluetooth/hci_core.c | 42 ++++++++++++++++++++++++++++++++++++++++
>> net/bluetooth/hci_event.c | 9 ++++++---
>> 4 files changed, 51 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>> index e56f909..2f0bce2 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -203,6 +203,7 @@ enum {
>> HCI_FAST_CONNECTABLE,
>> HCI_BREDR_ENABLED,
>> HCI_LE_SCAN_INTERRUPTED,
>> + HCI_LE_SCAN_RESTARTING,
>> };
>>
>> /* A mask for the flags that are supposed to remain when a reset happens
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index 396c098..36211f9 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -337,6 +337,7 @@ struct hci_dev {
>> unsigned long dev_flags;
>>
>> struct delayed_work le_scan_disable;
>> + struct delayed_work le_scan_restart;
>>
>> __s8 adv_tx_power;
>> __u8 adv_data[HCI_MAX_AD_LENGTH];
>> @@ -1403,6 +1404,7 @@ int hci_update_random_address(struct hci_request *req, bool require_privacy,
>> u8 *own_addr_type);
>> void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
>> u8 *bdaddr_type);
>> +void hci_restart_le_scan(struct hci_dev *hdev, unsigned long delay);
>>
>> #define SCO_AIRMODE_MASK 0x0003
>> #define SCO_AIRMODE_CVSD 0x0000
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 5c319a4..858cf54 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -2557,6 +2557,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
>> cancel_delayed_work(&hdev->service_cache);
>>
>> cancel_delayed_work_sync(&hdev->le_scan_disable);
>> + cancel_delayed_work_sync(&hdev->le_scan_restart);
>>
>> if (test_bit(HCI_MGMT, &hdev->dev_flags))
>> cancel_delayed_work_sync(&hdev->rpa_expired);
>> @@ -3851,6 +3852,8 @@ static void le_scan_disable_work(struct work_struct *work)
>>
>> BT_DBG("%s", hdev->name);
>>
>> + cancel_delayed_work_sync(&hdev->le_scan_restart);
>> +
>> hci_req_init(&req, hdev);
>>
>> hci_req_add_le_scan_disable(&req);
>> @@ -3860,6 +3863,44 @@ static void le_scan_disable_work(struct work_struct *work)
>> BT_ERR("Disable LE scanning request failed: err %d", err);
>> }
>>
>> +void hci_restart_le_scan(struct hci_dev *hdev, unsigned long delay)
>> +{
>> + queue_delayed_work(hdev->workqueue, &hdev->le_scan_restart, delay);
>> +}
>
> Since you're exposing this as a public API, does it make sense to
> check here if a delayed work has already been scheduled? I.e. is it OK
> if this function gets called several times in succession with
> different delay values?
Next patch introduces DISCOV_LE_RESTART_DELAY msecs_to_jiffies(300)
which is the only delay value used to schedule le_scan_restart right now.
I might change the implementation to:
void hci_restart_le_scan(struct hci_dev *hdev)
{
queue_delayed_work(hdev->workqueue, &hdev->le_scan_restart,
DISCOV_LE_RESTART_DELAY);
}
>
>> +
>> +static void le_scan_restart_work_complete(struct hci_dev *hdev, u8 status)
>> +{
>> + clear_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags);
>> +
>> + if (status)
>> + BT_ERR("Failed to restart LE scanning: status %d", status);
>> +}
>> +
>> +static void le_scan_restart_work(struct work_struct *work)
>> +{
>> + struct hci_dev *hdev = container_of(work, struct hci_dev,
>> + le_scan_restart.work);
>> + struct hci_request req;
>> + struct hci_cp_le_set_scan_enable cp;
>> + int err;
>> +
>> + BT_DBG("%s", hdev->name);
>> +
>> + hci_req_init(&req, hdev);
>> +
>> + hci_req_add_le_scan_disable(&req);
>> +
>> + memset(&cp, 0, sizeof(cp));
>> + cp.enable = LE_SCAN_ENABLE;
>> + hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
>> +
>> + set_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags);
>> +
>> + err = hci_req_run(&req, le_scan_restart_work_complete);
>> + if (err)
>> + BT_ERR("Disable LE scanning request failed: err %d", err);
>> +}
>> +
>> static void set_random_addr(struct hci_request *req, bdaddr_t *rpa)
>> {
>> struct hci_dev *hdev = req->hdev;
>> @@ -4037,6 +4078,7 @@ struct hci_dev *hci_alloc_dev(void)
>> INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
>> INIT_DELAYED_WORK(&hdev->discov_off, hci_discov_off);
>> INIT_DELAYED_WORK(&hdev->le_scan_disable, le_scan_disable_work);
>> + INIT_DELAYED_WORK(&hdev->le_scan_restart, le_scan_restart_work);
>>
>> skb_queue_head_init(&hdev->rx_q);
>> skb_queue_head_init(&hdev->cmd_q);
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index bd0a801..ed4d5e1 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -1157,10 +1157,13 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>> d->last_adv_data_len, NULL, 0);
>> }
>>
>> - /* Cancel this timer so that we don't try to disable scanning
>> - * when it's already disabled.
>> + /* If HCI_LE_SCAN_RESTARTING is set, don't cancel this timer,
>> + * because we're just restarting scan. Otherwise cancel it so
>> + * that we don't try to disable scanning when it's already
>> + * disabled.
>> */
>> - cancel_delayed_work(&hdev->le_scan_disable);
>> + if (!test_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags))
>> + cancel_delayed_work(&hdev->le_scan_disable);
>>
>> clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
>>
>> --
>> 2.1.0.rc2.206.gedb03e5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> Thanks,
> Arman
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 2/3] Bluetooth: Extract generic start and stop discovery
2014-11-20 22:14 ` Arman Uguray
@ 2014-11-20 23:14 ` Jakub Pawlowski
2014-11-20 23:40 ` Arman Uguray
0 siblings, 1 reply; 15+ messages in thread
From: Jakub Pawlowski @ 2014-11-20 23:14 UTC (permalink / raw)
To: Arman Uguray; +Cc: BlueZ development
On Thu, Nov 20, 2014 at 2:14 PM, Arman Uguray <armansito@chromium.org> wrote:
> Hi Jakub,
>
>> On Thu, Nov 20, 2014 at 9:51 AM, Jakub Pawlowski <jpawlowski@google.com> wrote:
>> This commit extract generic_start_discovery and generic_stop_discovery
>> in preparation for start and stop service discovery. The reason behind
>> that is that both functions will share big part of code, and it would
>> be much easier to maintain just one generic method.
>>
>> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
>> ---
>> net/bluetooth/mgmt.c | 147 ++++++++++++++++++++++++++++++---------------------
>> 1 file changed, 86 insertions(+), 61 deletions(-)
>>
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index cbeef5f..f650d30 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -3675,7 +3675,8 @@ done:
>> return err;
>> }
>>
>> -static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status)
>> +static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status,
>> + uint16_t opcode)
>> {
>> struct pending_cmd *cmd;
>> u8 type;
>> @@ -3683,7 +3684,7 @@ static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status)
>>
>> hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>>
>> - cmd = mgmt_pending_find(MGMT_OP_START_DISCOVERY, hdev);
>> + cmd = mgmt_pending_find(opcode, hdev);
>> if (!cmd)
>> return -ENOENT;
>>
>> @@ -3696,7 +3697,8 @@ static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status)
>> return err;
>> }
>>
>> -static void start_discovery_complete(struct hci_dev *hdev, u8 status)
>> +static void generic_start_discovery_complete(struct hci_dev *hdev, u8 status,
>> + uint16_t opcode)
>
> I'm not that big on calling these generic functions "generic_*". Let's
> try to come up with something better.
>
>> {
>> unsigned long timeout = 0;
>>
>> @@ -3704,7 +3706,7 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status)
>>
>> if (status) {
>> hci_dev_lock(hdev);
>> - mgmt_start_discovery_failed(hdev, status);
>> + mgmt_start_discovery_failed(hdev, status, opcode);
>> hci_dev_unlock(hdev);
>> return;
>> }
>> @@ -3735,8 +3737,13 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status)
>> queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable, timeout);
>> }
>>
>> -static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>> - void *data, u16 len)
>> +static void start_discovery_complete(struct hci_dev *hdev, u8 status)
>> +{
>> + generic_start_discovery_complete(hdev, status, MGMT_OP_START_DISCOVERY);
>> +}
>> +
>> +static int generic_start_discovery(struct sock *sk, struct hci_dev *hdev,
>> + void *data, u16 len, uint16_t opcode)
>> {
>> struct mgmt_cp_start_discovery *cp = data;
>> struct pending_cmd *cmd;
>> @@ -3746,41 +3753,41 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>> struct hci_request req;
>> /* General inquiry access code (GIAC) */
>> u8 lap[3] = { 0x33, 0x8b, 0x9e };
>> - u8 status, own_addr_type;
>> + u8 status, own_addr_type, type;
>> int err;
>>
>> BT_DBG("%s", hdev->name);
>>
>> + type = cp->type;
>> +
>
> I don't see the point of this local variable & assignment. Why not
> just leave it as it is, "cp->type" isn't that wordy.
So this patch is preparation for next patch which will have if/else
statement here that would pick proper type for different cases.
The next patch is already big so I wanted to make this change from
cp->type to type in this one. If that's a bad idea I'll move that to
next patch.
>
>> hci_dev_lock(hdev);
>>
>> if (!hdev_is_powered(hdev)) {
>> - err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
>> - MGMT_STATUS_NOT_POWERED,
>> - &cp->type, sizeof(cp->type));
>> + err = cmd_complete(sk, hdev->id, opcode,
>> + MGMT_STATUS_NOT_POWERED, &type,
>> + sizeof(type));
>> goto failed;
>> }
>>
>> if (test_bit(HCI_PERIODIC_INQ, &hdev->dev_flags)) {
>> - err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
>> - MGMT_STATUS_BUSY, &cp->type,
>> - sizeof(cp->type));
>> + err = cmd_complete(sk, hdev->id, opcode,
>> + MGMT_STATUS_BUSY, &type, sizeof(type));
>> goto failed;
>> }
>>
>> if (hdev->discovery.state != DISCOVERY_STOPPED) {
>> - err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
>> - MGMT_STATUS_BUSY, &cp->type,
>> - sizeof(cp->type));
>> + err = cmd_complete(sk, hdev->id, opcode,
>> + MGMT_STATUS_BUSY, &type, sizeof(type));
>> goto failed;
>> }
>>
>> - cmd = mgmt_pending_add(sk, MGMT_OP_START_DISCOVERY, hdev, NULL, 0);
>> + cmd = mgmt_pending_add(sk, opcode, hdev, NULL, 0);
>> if (!cmd) {
>> err = -ENOMEM;
>> goto failed;
>> }
>>
>> - hdev->discovery.type = cp->type;
>> + hdev->discovery.type = type;
>>
>> hci_req_init(&req, hdev);
>>
>> @@ -3788,18 +3795,16 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>> case DISCOV_TYPE_BREDR:
>> status = mgmt_bredr_support(hdev);
>> if (status) {
>> - err = cmd_complete(sk, hdev->id,
>> - MGMT_OP_START_DISCOVERY, status,
>> - &cp->type, sizeof(cp->type));
>> + err = cmd_complete(sk, hdev->id, opcode, status, &type,
>> + sizeof(type));
>> mgmt_pending_remove(cmd);
>> goto failed;
>> }
>>
>> if (test_bit(HCI_INQUIRY, &hdev->flags)) {
>> - err = cmd_complete(sk, hdev->id,
>> - MGMT_OP_START_DISCOVERY,
>> - MGMT_STATUS_BUSY, &cp->type,
>> - sizeof(cp->type));
>> + err = cmd_complete(sk, hdev->id, opcode,
>> + MGMT_STATUS_BUSY, &type,
>> + sizeof(type));
>> mgmt_pending_remove(cmd);
>> goto failed;
>> }
>> @@ -3816,19 +3821,17 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>> case DISCOV_TYPE_INTERLEAVED:
>> status = mgmt_le_support(hdev);
>> if (status) {
>> - err = cmd_complete(sk, hdev->id,
>> - MGMT_OP_START_DISCOVERY, status,
>> - &cp->type, sizeof(cp->type));
>> + err = cmd_complete(sk, hdev->id, opcode, status, &type,
>> + sizeof(type));
>> mgmt_pending_remove(cmd);
>> goto failed;
>> }
>>
>> if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
>> !test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
>> - err = cmd_complete(sk, hdev->id,
>> - MGMT_OP_START_DISCOVERY,
>> - MGMT_STATUS_NOT_SUPPORTED,
>> - &cp->type, sizeof(cp->type));
>> + err = cmd_complete(sk, hdev->id, opcode,
>> + MGMT_STATUS_NOT_SUPPORTED, &type,
>> + sizeof(type));
>> mgmt_pending_remove(cmd);
>> goto failed;
>> }
>> @@ -3840,11 +3843,9 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>> */
>> if (hci_conn_hash_lookup_state(hdev, LE_LINK,
>> BT_CONNECT)) {
>> - err = cmd_complete(sk, hdev->id,
>> - MGMT_OP_START_DISCOVERY,
>> - MGMT_STATUS_REJECTED,
>> - &cp->type,
>> - sizeof(cp->type));
>> + err = cmd_complete(sk, hdev->id, opcode,
>> + MGMT_STATUS_REJECTED, &type,
>> + sizeof(type));
>> mgmt_pending_remove(cmd);
>> goto failed;
>> }
>> @@ -3867,10 +3868,9 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>> */
>> err = hci_update_random_address(&req, true, &own_addr_type);
>> if (err < 0) {
>> - err = cmd_complete(sk, hdev->id,
>> - MGMT_OP_START_DISCOVERY,
>> - MGMT_STATUS_FAILED,
>> - &cp->type, sizeof(cp->type));
>> + err = cmd_complete(sk, hdev->id, opcode,
>> + MGMT_STATUS_FAILED, &type,
>> + sizeof(type));
>> mgmt_pending_remove(cmd);
>> goto failed;
>> }
>> @@ -3890,14 +3890,15 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>> break;
>>
>> default:
>> - err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
>> - MGMT_STATUS_INVALID_PARAMS,
>> - &cp->type, sizeof(cp->type));
>> + err = cmd_complete(sk, hdev->id, opcode,
>> + MGMT_STATUS_INVALID_PARAMS, &type,
>> + sizeof(type));
>> mgmt_pending_remove(cmd);
>> goto failed;
>> }
>>
>> err = hci_req_run(&req, start_discovery_complete);
>> +
>> if (err < 0)
>> mgmt_pending_remove(cmd);
>> else
>> @@ -3908,12 +3909,20 @@ failed:
>> return err;
>> }
>>
>> -static int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status)
>> +static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>> + void *data, u16 len)
>> +{
>> + return generic_start_discovery(sk, hdev, data, len,
>> + MGMT_OP_START_DISCOVERY);
>> +}
>> +
>> +static int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status,
>> + uint16_t opcode)
>> {
>> struct pending_cmd *cmd;
>> int err;
>>
>> - cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, hdev);
>> + cmd = mgmt_pending_find(opcode, hdev);
>> if (!cmd)
>> return -ENOENT;
>>
>> @@ -3924,14 +3933,15 @@ static int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status)
>> return err;
>> }
>>
>> -static void stop_discovery_complete(struct hci_dev *hdev, u8 status)
>> +static void generic_stop_discovery_complete(struct hci_dev *hdev, u8 status,
>> + uint16_t opcode)
>> {
>> BT_DBG("status %d", status);
>>
>> hci_dev_lock(hdev);
>>
>> if (status) {
>> - mgmt_stop_discovery_failed(hdev, status);
>> + mgmt_stop_discovery_failed(hdev, status, opcode);
>> goto unlock;
>> }
>>
>> @@ -3941,33 +3951,40 @@ unlock:
>> hci_dev_unlock(hdev);
>> }
>>
>> -static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>> - u16 len)
>> +static void stop_discovery_complete(struct hci_dev *hdev, u8 status)
>> +{
>> + generic_stop_discovery_complete(hdev, status, MGMT_OP_STOP_DISCOVERY);
>> +}
>> +
>> +static int generic_stop_discovery(struct sock *sk, struct hci_dev *hdev,
>> + void *data, u16 len, uint16_t opcode)
>> {
>> - struct mgmt_cp_stop_discovery *mgmt_cp = data;
>> struct pending_cmd *cmd;
>> struct hci_request req;
>> + struct mgmt_cp_stop_discovery *mgmt_cp = data;
>
> Why move this?
I'll fix that
>
>> int err;
>> + u8 type;
>>
>
> Again, there's no need for this variable.
same as aboce - this patch is preparation for next patch which will
have if/else statement here that would pick proper type for different
cases.
>
>> BT_DBG("%s", hdev->name);
>>
>> + type = mgmt_cp->type;
>> +
>> hci_dev_lock(hdev);
>>
>> if (!hci_discovery_active(hdev)) {
>> - err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY,
>> - MGMT_STATUS_REJECTED, &mgmt_cp->type,
>> - sizeof(mgmt_cp->type));
>> + err = cmd_complete(sk, hdev->id, opcode,
>> + MGMT_STATUS_REJECTED, &type, sizeof(type));
>> goto unlock;
>> }
>>
>> - if (hdev->discovery.type != mgmt_cp->type) {
>> - err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY,
>> - MGMT_STATUS_INVALID_PARAMS, &mgmt_cp->type,
>> - sizeof(mgmt_cp->type));
>> + if (hdev->discovery.type != type) {
>> + err = cmd_complete(sk, hdev->id, opcode,
>> + MGMT_STATUS_INVALID_PARAMS, &type,
>> + sizeof(type));
>> goto unlock;
>> }
>>
>> - cmd = mgmt_pending_add(sk, MGMT_OP_STOP_DISCOVERY, hdev, NULL, 0);
>> + cmd = mgmt_pending_add(sk, opcode, hdev, NULL, 0);
>> if (!cmd) {
>> err = -ENOMEM;
>> goto unlock;
>> @@ -3978,6 +3995,7 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>> hci_stop_discovery(&req);
>>
>> err = hci_req_run(&req, stop_discovery_complete);
>> +
>> if (!err) {
>> hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
>> goto unlock;
>> @@ -3987,8 +4005,8 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>>
>> /* If no HCI commands were sent we're done */
>> if (err == -ENODATA) {
>> - err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY, 0,
>> - &mgmt_cp->type, sizeof(mgmt_cp->type));
>> + err = cmd_complete(sk, hdev->id, opcode, 0,
>> + &type, sizeof(type));
>> hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>> }
>>
>> @@ -3997,6 +4015,13 @@ unlock:
>> return err;
>> }
>>
>> +static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>> + u16 len)
>> +{
>> + return generic_stop_discovery(sk, hdev, data, len,
>> + MGMT_OP_STOP_DISCOVERY);
>> +}
>> +
>> static int confirm_name(struct sock *sk, struct hci_dev *hdev, void *data,
>> u16 len)
>> {
>> --
>> 2.1.0.rc2.206.gedb03e5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> Overall I think this patch looks good, except for a few nits that I
> commented on above.
>
> Thanks,
> Arman
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 1/3] Bluetooth: add hci_restart_le_scan
2014-11-20 22:24 ` Jakub Pawlowski
@ 2014-11-20 23:37 ` Arman Uguray
2014-11-20 23:54 ` Jakub Pawlowski
0 siblings, 1 reply; 15+ messages in thread
From: Arman Uguray @ 2014-11-20 23:37 UTC (permalink / raw)
To: Jakub Pawlowski; +Cc: BlueZ development
Hi Jakub,
> On Thu, Nov 20, 2014 at 2:24 PM, Jakub Pawlowski <jpawlowski@google.com> wrote:
> On Thu, Nov 20, 2014 at 1:48 PM, Arman Uguray <armansito@chromium.org> wrote:
>> Hi Jakub,
>>
>>> On Thu, Nov 20, 2014 at 9:51 AM, Jakub Pawlowski <jpawlowski@google.com> wrote:
>>> Currently there is no way to restart le scan. It's needed in
>>> preparation for new service scan method. The way it work: it disable,
>>> and then enable le scan on controller. During this restart special flag
>>> is set to make sure we won't remove disable scan work from workqueue.
>>>
>>> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
>>> ---
>>> include/net/bluetooth/hci.h | 1 +
>>> include/net/bluetooth/hci_core.h | 2 ++
>>> net/bluetooth/hci_core.c | 42 ++++++++++++++++++++++++++++++++++++++++
>>> net/bluetooth/hci_event.c | 9 ++++++---
>>> 4 files changed, 51 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>>> index e56f909..2f0bce2 100644
>>> --- a/include/net/bluetooth/hci.h
>>> +++ b/include/net/bluetooth/hci.h
>>> @@ -203,6 +203,7 @@ enum {
>>> HCI_FAST_CONNECTABLE,
>>> HCI_BREDR_ENABLED,
>>> HCI_LE_SCAN_INTERRUPTED,
>>> + HCI_LE_SCAN_RESTARTING,
>>> };
>>>
>>> /* A mask for the flags that are supposed to remain when a reset happens
>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>> index 396c098..36211f9 100644
>>> --- a/include/net/bluetooth/hci_core.h
>>> +++ b/include/net/bluetooth/hci_core.h
>>> @@ -337,6 +337,7 @@ struct hci_dev {
>>> unsigned long dev_flags;
>>>
>>> struct delayed_work le_scan_disable;
>>> + struct delayed_work le_scan_restart;
>>>
>>> __s8 adv_tx_power;
>>> __u8 adv_data[HCI_MAX_AD_LENGTH];
>>> @@ -1403,6 +1404,7 @@ int hci_update_random_address(struct hci_request *req, bool require_privacy,
>>> u8 *own_addr_type);
>>> void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
>>> u8 *bdaddr_type);
>>> +void hci_restart_le_scan(struct hci_dev *hdev, unsigned long delay);
>>>
>>> #define SCO_AIRMODE_MASK 0x0003
>>> #define SCO_AIRMODE_CVSD 0x0000
>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>> index 5c319a4..858cf54 100644
>>> --- a/net/bluetooth/hci_core.c
>>> +++ b/net/bluetooth/hci_core.c
>>> @@ -2557,6 +2557,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
>>> cancel_delayed_work(&hdev->service_cache);
>>>
>>> cancel_delayed_work_sync(&hdev->le_scan_disable);
>>> + cancel_delayed_work_sync(&hdev->le_scan_restart);
>>>
>>> if (test_bit(HCI_MGMT, &hdev->dev_flags))
>>> cancel_delayed_work_sync(&hdev->rpa_expired);
>>> @@ -3851,6 +3852,8 @@ static void le_scan_disable_work(struct work_struct *work)
>>>
>>> BT_DBG("%s", hdev->name);
>>>
>>> + cancel_delayed_work_sync(&hdev->le_scan_restart);
>>> +
>>> hci_req_init(&req, hdev);
>>>
>>> hci_req_add_le_scan_disable(&req);
>>> @@ -3860,6 +3863,44 @@ static void le_scan_disable_work(struct work_struct *work)
>>> BT_ERR("Disable LE scanning request failed: err %d", err);
>>> }
>>>
>>> +void hci_restart_le_scan(struct hci_dev *hdev, unsigned long delay)
>>> +{
>>> + queue_delayed_work(hdev->workqueue, &hdev->le_scan_restart, delay);
>>> +}
>>
>> Since you're exposing this as a public API, does it make sense to
>> check here if a delayed work has already been scheduled? I.e. is it OK
>> if this function gets called several times in succession with
>> different delay values?
>
> Next patch introduces DISCOV_LE_RESTART_DELAY msecs_to_jiffies(300)
> which is the only delay value used to schedule le_scan_restart right now.
> I might change the implementation to:
>
> void hci_restart_le_scan(struct hci_dev *hdev)
> {
> queue_delayed_work(hdev->workqueue, &hdev->le_scan_restart,
> DISCOV_LE_RESTART_DELAY);
> }
>
Sure but even with a consistent delay value, is it OK to repeatedly
call this function from other layers? Since this is publicly exposed
from hci_core.h, any code within the subsystem can make calls to this.
So you should make sure that it doesn't break this, or cover that edge
case inside this function, if necessary.
>>
>>> +
>>> +static void le_scan_restart_work_complete(struct hci_dev *hdev, u8 status)
>>> +{
>>> + clear_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags);
>>> +
>>> + if (status)
>>> + BT_ERR("Failed to restart LE scanning: status %d", status);
>>> +}
>>> +
>>> +static void le_scan_restart_work(struct work_struct *work)
>>> +{
>>> + struct hci_dev *hdev = container_of(work, struct hci_dev,
>>> + le_scan_restart.work);
>>> + struct hci_request req;
>>> + struct hci_cp_le_set_scan_enable cp;
>>> + int err;
>>> +
>>> + BT_DBG("%s", hdev->name);
>>> +
>>> + hci_req_init(&req, hdev);
>>> +
>>> + hci_req_add_le_scan_disable(&req);
>>> +
>>> + memset(&cp, 0, sizeof(cp));
>>> + cp.enable = LE_SCAN_ENABLE;
>>> + hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
>>> +
>>> + set_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags);
>>> +
>>> + err = hci_req_run(&req, le_scan_restart_work_complete);
>>> + if (err)
>>> + BT_ERR("Disable LE scanning request failed: err %d", err);
>>> +}
>>> +
>>> static void set_random_addr(struct hci_request *req, bdaddr_t *rpa)
>>> {
>>> struct hci_dev *hdev = req->hdev;
>>> @@ -4037,6 +4078,7 @@ struct hci_dev *hci_alloc_dev(void)
>>> INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
>>> INIT_DELAYED_WORK(&hdev->discov_off, hci_discov_off);
>>> INIT_DELAYED_WORK(&hdev->le_scan_disable, le_scan_disable_work);
>>> + INIT_DELAYED_WORK(&hdev->le_scan_restart, le_scan_restart_work);
>>>
>>> skb_queue_head_init(&hdev->rx_q);
>>> skb_queue_head_init(&hdev->cmd_q);
>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>> index bd0a801..ed4d5e1 100644
>>> --- a/net/bluetooth/hci_event.c
>>> +++ b/net/bluetooth/hci_event.c
>>> @@ -1157,10 +1157,13 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>>> d->last_adv_data_len, NULL, 0);
>>> }
>>>
>>> - /* Cancel this timer so that we don't try to disable scanning
>>> - * when it's already disabled.
>>> + /* If HCI_LE_SCAN_RESTARTING is set, don't cancel this timer,
>>> + * because we're just restarting scan. Otherwise cancel it so
>>> + * that we don't try to disable scanning when it's already
>>> + * disabled.
>>> */
>>> - cancel_delayed_work(&hdev->le_scan_disable);
>>> + if (!test_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags))
>>> + cancel_delayed_work(&hdev->le_scan_disable);
>>>
>>> clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
>>>
>>> --
>>> 2.1.0.rc2.206.gedb03e5
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> Thanks,
>> Arman
Arman
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 2/3] Bluetooth: Extract generic start and stop discovery
2014-11-20 23:14 ` Jakub Pawlowski
@ 2014-11-20 23:40 ` Arman Uguray
2014-11-20 23:55 ` Jakub Pawlowski
0 siblings, 1 reply; 15+ messages in thread
From: Arman Uguray @ 2014-11-20 23:40 UTC (permalink / raw)
To: Jakub Pawlowski; +Cc: BlueZ development
Hi Jakub,
> On Thu, Nov 20, 2014 at 3:14 PM, Jakub Pawlowski <jpawlowski@google.com> wrote:
> On Thu, Nov 20, 2014 at 2:14 PM, Arman Uguray <armansito@chromium.org> wrote:
>> Hi Jakub,
>>
>>> On Thu, Nov 20, 2014 at 9:51 AM, Jakub Pawlowski <jpawlowski@google.com> wrote:
>>> This commit extract generic_start_discovery and generic_stop_discovery
>>> in preparation for start and stop service discovery. The reason behind
>>> that is that both functions will share big part of code, and it would
>>> be much easier to maintain just one generic method.
>>>
>>> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
>>> ---
>>> net/bluetooth/mgmt.c | 147 ++++++++++++++++++++++++++++++---------------------
>>> 1 file changed, 86 insertions(+), 61 deletions(-)
>>>
>>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>>> index cbeef5f..f650d30 100644
>>> --- a/net/bluetooth/mgmt.c
>>> +++ b/net/bluetooth/mgmt.c
>>> @@ -3675,7 +3675,8 @@ done:
>>> return err;
>>> }
>>>
>>> -static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status)
>>> +static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status,
>>> + uint16_t opcode)
>>> {
>>> struct pending_cmd *cmd;
>>> u8 type;
>>> @@ -3683,7 +3684,7 @@ static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status)
>>>
>>> hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>>>
>>> - cmd = mgmt_pending_find(MGMT_OP_START_DISCOVERY, hdev);
>>> + cmd = mgmt_pending_find(opcode, hdev);
>>> if (!cmd)
>>> return -ENOENT;
>>>
>>> @@ -3696,7 +3697,8 @@ static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status)
>>> return err;
>>> }
>>>
>>> -static void start_discovery_complete(struct hci_dev *hdev, u8 status)
>>> +static void generic_start_discovery_complete(struct hci_dev *hdev, u8 status,
>>> + uint16_t opcode)
>>
>> I'm not that big on calling these generic functions "generic_*". Let's
>> try to come up with something better.
>>
>>> {
>>> unsigned long timeout = 0;
>>>
>>> @@ -3704,7 +3706,7 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status)
>>>
>>> if (status) {
>>> hci_dev_lock(hdev);
>>> - mgmt_start_discovery_failed(hdev, status);
>>> + mgmt_start_discovery_failed(hdev, status, opcode);
>>> hci_dev_unlock(hdev);
>>> return;
>>> }
>>> @@ -3735,8 +3737,13 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status)
>>> queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable, timeout);
>>> }
>>>
>>> -static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>>> - void *data, u16 len)
>>> +static void start_discovery_complete(struct hci_dev *hdev, u8 status)
>>> +{
>>> + generic_start_discovery_complete(hdev, status, MGMT_OP_START_DISCOVERY);
>>> +}
>>> +
>>> +static int generic_start_discovery(struct sock *sk, struct hci_dev *hdev,
>>> + void *data, u16 len, uint16_t opcode)
>>> {
>>> struct mgmt_cp_start_discovery *cp = data;
>>> struct pending_cmd *cmd;
>>> @@ -3746,41 +3753,41 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>>> struct hci_request req;
>>> /* General inquiry access code (GIAC) */
>>> u8 lap[3] = { 0x33, 0x8b, 0x9e };
>>> - u8 status, own_addr_type;
>>> + u8 status, own_addr_type, type;
>>> int err;
>>>
>>> BT_DBG("%s", hdev->name);
>>>
>>> + type = cp->type;
>>> +
>>
>> I don't see the point of this local variable & assignment. Why not
>> just leave it as it is, "cp->type" isn't that wordy.
>
> So this patch is preparation for next patch which will have if/else
> statement here that would pick proper type for different cases.
> The next patch is already big so I wanted to make this change from
> cp->type to type in this one. If that's a bad idea I'll move that to
> next patch.
>
Yeah, from here it just looks like a random refactor. I would just put
it into the next patch. Or if you think that the next patch is too
big, maybe think about how you can make it smaller by separating the
logical steps?
>>
>>> hci_dev_lock(hdev);
>>>
>>> if (!hdev_is_powered(hdev)) {
>>> - err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
>>> - MGMT_STATUS_NOT_POWERED,
>>> - &cp->type, sizeof(cp->type));
>>> + err = cmd_complete(sk, hdev->id, opcode,
>>> + MGMT_STATUS_NOT_POWERED, &type,
>>> + sizeof(type));
>>> goto failed;
>>> }
>>>
>>> if (test_bit(HCI_PERIODIC_INQ, &hdev->dev_flags)) {
>>> - err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
>>> - MGMT_STATUS_BUSY, &cp->type,
>>> - sizeof(cp->type));
>>> + err = cmd_complete(sk, hdev->id, opcode,
>>> + MGMT_STATUS_BUSY, &type, sizeof(type));
>>> goto failed;
>>> }
>>>
>>> if (hdev->discovery.state != DISCOVERY_STOPPED) {
>>> - err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
>>> - MGMT_STATUS_BUSY, &cp->type,
>>> - sizeof(cp->type));
>>> + err = cmd_complete(sk, hdev->id, opcode,
>>> + MGMT_STATUS_BUSY, &type, sizeof(type));
>>> goto failed;
>>> }
>>>
>>> - cmd = mgmt_pending_add(sk, MGMT_OP_START_DISCOVERY, hdev, NULL, 0);
>>> + cmd = mgmt_pending_add(sk, opcode, hdev, NULL, 0);
>>> if (!cmd) {
>>> err = -ENOMEM;
>>> goto failed;
>>> }
>>>
>>> - hdev->discovery.type = cp->type;
>>> + hdev->discovery.type = type;
>>>
>>> hci_req_init(&req, hdev);
>>>
>>> @@ -3788,18 +3795,16 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>>> case DISCOV_TYPE_BREDR:
>>> status = mgmt_bredr_support(hdev);
>>> if (status) {
>>> - err = cmd_complete(sk, hdev->id,
>>> - MGMT_OP_START_DISCOVERY, status,
>>> - &cp->type, sizeof(cp->type));
>>> + err = cmd_complete(sk, hdev->id, opcode, status, &type,
>>> + sizeof(type));
>>> mgmt_pending_remove(cmd);
>>> goto failed;
>>> }
>>>
>>> if (test_bit(HCI_INQUIRY, &hdev->flags)) {
>>> - err = cmd_complete(sk, hdev->id,
>>> - MGMT_OP_START_DISCOVERY,
>>> - MGMT_STATUS_BUSY, &cp->type,
>>> - sizeof(cp->type));
>>> + err = cmd_complete(sk, hdev->id, opcode,
>>> + MGMT_STATUS_BUSY, &type,
>>> + sizeof(type));
>>> mgmt_pending_remove(cmd);
>>> goto failed;
>>> }
>>> @@ -3816,19 +3821,17 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>>> case DISCOV_TYPE_INTERLEAVED:
>>> status = mgmt_le_support(hdev);
>>> if (status) {
>>> - err = cmd_complete(sk, hdev->id,
>>> - MGMT_OP_START_DISCOVERY, status,
>>> - &cp->type, sizeof(cp->type));
>>> + err = cmd_complete(sk, hdev->id, opcode, status, &type,
>>> + sizeof(type));
>>> mgmt_pending_remove(cmd);
>>> goto failed;
>>> }
>>>
>>> if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
>>> !test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
>>> - err = cmd_complete(sk, hdev->id,
>>> - MGMT_OP_START_DISCOVERY,
>>> - MGMT_STATUS_NOT_SUPPORTED,
>>> - &cp->type, sizeof(cp->type));
>>> + err = cmd_complete(sk, hdev->id, opcode,
>>> + MGMT_STATUS_NOT_SUPPORTED, &type,
>>> + sizeof(type));
>>> mgmt_pending_remove(cmd);
>>> goto failed;
>>> }
>>> @@ -3840,11 +3843,9 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>>> */
>>> if (hci_conn_hash_lookup_state(hdev, LE_LINK,
>>> BT_CONNECT)) {
>>> - err = cmd_complete(sk, hdev->id,
>>> - MGMT_OP_START_DISCOVERY,
>>> - MGMT_STATUS_REJECTED,
>>> - &cp->type,
>>> - sizeof(cp->type));
>>> + err = cmd_complete(sk, hdev->id, opcode,
>>> + MGMT_STATUS_REJECTED, &type,
>>> + sizeof(type));
>>> mgmt_pending_remove(cmd);
>>> goto failed;
>>> }
>>> @@ -3867,10 +3868,9 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>>> */
>>> err = hci_update_random_address(&req, true, &own_addr_type);
>>> if (err < 0) {
>>> - err = cmd_complete(sk, hdev->id,
>>> - MGMT_OP_START_DISCOVERY,
>>> - MGMT_STATUS_FAILED,
>>> - &cp->type, sizeof(cp->type));
>>> + err = cmd_complete(sk, hdev->id, opcode,
>>> + MGMT_STATUS_FAILED, &type,
>>> + sizeof(type));
>>> mgmt_pending_remove(cmd);
>>> goto failed;
>>> }
>>> @@ -3890,14 +3890,15 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>>> break;
>>>
>>> default:
>>> - err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
>>> - MGMT_STATUS_INVALID_PARAMS,
>>> - &cp->type, sizeof(cp->type));
>>> + err = cmd_complete(sk, hdev->id, opcode,
>>> + MGMT_STATUS_INVALID_PARAMS, &type,
>>> + sizeof(type));
>>> mgmt_pending_remove(cmd);
>>> goto failed;
>>> }
>>>
>>> err = hci_req_run(&req, start_discovery_complete);
>>> +
>>> if (err < 0)
>>> mgmt_pending_remove(cmd);
>>> else
>>> @@ -3908,12 +3909,20 @@ failed:
>>> return err;
>>> }
>>>
>>> -static int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status)
>>> +static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>>> + void *data, u16 len)
>>> +{
>>> + return generic_start_discovery(sk, hdev, data, len,
>>> + MGMT_OP_START_DISCOVERY);
>>> +}
>>> +
>>> +static int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status,
>>> + uint16_t opcode)
>>> {
>>> struct pending_cmd *cmd;
>>> int err;
>>>
>>> - cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, hdev);
>>> + cmd = mgmt_pending_find(opcode, hdev);
>>> if (!cmd)
>>> return -ENOENT;
>>>
>>> @@ -3924,14 +3933,15 @@ static int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status)
>>> return err;
>>> }
>>>
>>> -static void stop_discovery_complete(struct hci_dev *hdev, u8 status)
>>> +static void generic_stop_discovery_complete(struct hci_dev *hdev, u8 status,
>>> + uint16_t opcode)
>>> {
>>> BT_DBG("status %d", status);
>>>
>>> hci_dev_lock(hdev);
>>>
>>> if (status) {
>>> - mgmt_stop_discovery_failed(hdev, status);
>>> + mgmt_stop_discovery_failed(hdev, status, opcode);
>>> goto unlock;
>>> }
>>>
>>> @@ -3941,33 +3951,40 @@ unlock:
>>> hci_dev_unlock(hdev);
>>> }
>>>
>>> -static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>>> - u16 len)
>>> +static void stop_discovery_complete(struct hci_dev *hdev, u8 status)
>>> +{
>>> + generic_stop_discovery_complete(hdev, status, MGMT_OP_STOP_DISCOVERY);
>>> +}
>>> +
>>> +static int generic_stop_discovery(struct sock *sk, struct hci_dev *hdev,
>>> + void *data, u16 len, uint16_t opcode)
>>> {
>>> - struct mgmt_cp_stop_discovery *mgmt_cp = data;
>>> struct pending_cmd *cmd;
>>> struct hci_request req;
>>> + struct mgmt_cp_stop_discovery *mgmt_cp = data;
>>
>> Why move this?
>
> I'll fix that
>>
>>> int err;
>>> + u8 type;
>>>
>>
>> Again, there's no need for this variable.
>
> same as aboce - this patch is preparation for next patch which will
> have if/else statement here that would pick proper type for different
> cases.
>
>>
>>> BT_DBG("%s", hdev->name);
>>>
>>> + type = mgmt_cp->type;
>>> +
>>> hci_dev_lock(hdev);
>>>
>>> if (!hci_discovery_active(hdev)) {
>>> - err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY,
>>> - MGMT_STATUS_REJECTED, &mgmt_cp->type,
>>> - sizeof(mgmt_cp->type));
>>> + err = cmd_complete(sk, hdev->id, opcode,
>>> + MGMT_STATUS_REJECTED, &type, sizeof(type));
>>> goto unlock;
>>> }
>>>
>>> - if (hdev->discovery.type != mgmt_cp->type) {
>>> - err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY,
>>> - MGMT_STATUS_INVALID_PARAMS, &mgmt_cp->type,
>>> - sizeof(mgmt_cp->type));
>>> + if (hdev->discovery.type != type) {
>>> + err = cmd_complete(sk, hdev->id, opcode,
>>> + MGMT_STATUS_INVALID_PARAMS, &type,
>>> + sizeof(type));
>>> goto unlock;
>>> }
>>>
>>> - cmd = mgmt_pending_add(sk, MGMT_OP_STOP_DISCOVERY, hdev, NULL, 0);
>>> + cmd = mgmt_pending_add(sk, opcode, hdev, NULL, 0);
>>> if (!cmd) {
>>> err = -ENOMEM;
>>> goto unlock;
>>> @@ -3978,6 +3995,7 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>>> hci_stop_discovery(&req);
>>>
>>> err = hci_req_run(&req, stop_discovery_complete);
>>> +
>>> if (!err) {
>>> hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
>>> goto unlock;
>>> @@ -3987,8 +4005,8 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>>>
>>> /* If no HCI commands were sent we're done */
>>> if (err == -ENODATA) {
>>> - err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY, 0,
>>> - &mgmt_cp->type, sizeof(mgmt_cp->type));
>>> + err = cmd_complete(sk, hdev->id, opcode, 0,
>>> + &type, sizeof(type));
>>> hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>>> }
>>>
>>> @@ -3997,6 +4015,13 @@ unlock:
>>> return err;
>>> }
>>>
>>> +static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>>> + u16 len)
>>> +{
>>> + return generic_stop_discovery(sk, hdev, data, len,
>>> + MGMT_OP_STOP_DISCOVERY);
>>> +}
>>> +
>>> static int confirm_name(struct sock *sk, struct hci_dev *hdev, void *data,
>>> u16 len)
>>> {
>>> --
>>> 2.1.0.rc2.206.gedb03e5
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> Overall I think this patch looks good, except for a few nits that I
>> commented on above.
>>
>> Thanks,
>> Arman
Thanks,
Arman
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 1/3] Bluetooth: add hci_restart_le_scan
2014-11-20 23:37 ` Arman Uguray
@ 2014-11-20 23:54 ` Jakub Pawlowski
2014-11-20 23:56 ` Arman Uguray
0 siblings, 1 reply; 15+ messages in thread
From: Jakub Pawlowski @ 2014-11-20 23:54 UTC (permalink / raw)
To: Arman Uguray; +Cc: BlueZ development
On Thu, Nov 20, 2014 at 3:37 PM, Arman Uguray <armansito@chromium.org> wrote:
> Hi Jakub,
>
>> On Thu, Nov 20, 2014 at 2:24 PM, Jakub Pawlowski <jpawlowski@google.com> wrote:
>> On Thu, Nov 20, 2014 at 1:48 PM, Arman Uguray <armansito@chromium.org> wrote:
>>> Hi Jakub,
>>>
>>>> On Thu, Nov 20, 2014 at 9:51 AM, Jakub Pawlowski <jpawlowski@google.com> wrote:
>>>> Currently there is no way to restart le scan. It's needed in
>>>> preparation for new service scan method. The way it work: it disable,
>>>> and then enable le scan on controller. During this restart special flag
>>>> is set to make sure we won't remove disable scan work from workqueue.
>>>>
>>>> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
>>>> ---
>>>> include/net/bluetooth/hci.h | 1 +
>>>> include/net/bluetooth/hci_core.h | 2 ++
>>>> net/bluetooth/hci_core.c | 42 ++++++++++++++++++++++++++++++++++++++++
>>>> net/bluetooth/hci_event.c | 9 ++++++---
>>>> 4 files changed, 51 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>>>> index e56f909..2f0bce2 100644
>>>> --- a/include/net/bluetooth/hci.h
>>>> +++ b/include/net/bluetooth/hci.h
>>>> @@ -203,6 +203,7 @@ enum {
>>>> HCI_FAST_CONNECTABLE,
>>>> HCI_BREDR_ENABLED,
>>>> HCI_LE_SCAN_INTERRUPTED,
>>>> + HCI_LE_SCAN_RESTARTING,
>>>> };
>>>>
>>>> /* A mask for the flags that are supposed to remain when a reset happens
>>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>>> index 396c098..36211f9 100644
>>>> --- a/include/net/bluetooth/hci_core.h
>>>> +++ b/include/net/bluetooth/hci_core.h
>>>> @@ -337,6 +337,7 @@ struct hci_dev {
>>>> unsigned long dev_flags;
>>>>
>>>> struct delayed_work le_scan_disable;
>>>> + struct delayed_work le_scan_restart;
>>>>
>>>> __s8 adv_tx_power;
>>>> __u8 adv_data[HCI_MAX_AD_LENGTH];
>>>> @@ -1403,6 +1404,7 @@ int hci_update_random_address(struct hci_request *req, bool require_privacy,
>>>> u8 *own_addr_type);
>>>> void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
>>>> u8 *bdaddr_type);
>>>> +void hci_restart_le_scan(struct hci_dev *hdev, unsigned long delay);
>>>>
>>>> #define SCO_AIRMODE_MASK 0x0003
>>>> #define SCO_AIRMODE_CVSD 0x0000
>>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>>> index 5c319a4..858cf54 100644
>>>> --- a/net/bluetooth/hci_core.c
>>>> +++ b/net/bluetooth/hci_core.c
>>>> @@ -2557,6 +2557,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
>>>> cancel_delayed_work(&hdev->service_cache);
>>>>
>>>> cancel_delayed_work_sync(&hdev->le_scan_disable);
>>>> + cancel_delayed_work_sync(&hdev->le_scan_restart);
>>>>
>>>> if (test_bit(HCI_MGMT, &hdev->dev_flags))
>>>> cancel_delayed_work_sync(&hdev->rpa_expired);
>>>> @@ -3851,6 +3852,8 @@ static void le_scan_disable_work(struct work_struct *work)
>>>>
>>>> BT_DBG("%s", hdev->name);
>>>>
>>>> + cancel_delayed_work_sync(&hdev->le_scan_restart);
>>>> +
>>>> hci_req_init(&req, hdev);
>>>>
>>>> hci_req_add_le_scan_disable(&req);
>>>> @@ -3860,6 +3863,44 @@ static void le_scan_disable_work(struct work_struct *work)
>>>> BT_ERR("Disable LE scanning request failed: err %d", err);
>>>> }
>>>>
>>>> +void hci_restart_le_scan(struct hci_dev *hdev, unsigned long delay)
>>>> +{
>>>> + queue_delayed_work(hdev->workqueue, &hdev->le_scan_restart, delay);
>>>> +}
>>>
>>> Since you're exposing this as a public API, does it make sense to
>>> check here if a delayed work has already been scheduled? I.e. is it OK
>>> if this function gets called several times in succession with
>>> different delay values?
>>
>> Next patch introduces DISCOV_LE_RESTART_DELAY msecs_to_jiffies(300)
>> which is the only delay value used to schedule le_scan_restart right now.
>> I might change the implementation to:
>>
>> void hci_restart_le_scan(struct hci_dev *hdev)
>> {
>> queue_delayed_work(hdev->workqueue, &hdev->le_scan_restart,
>> DISCOV_LE_RESTART_DELAY);
>> }
>>
>
> Sure but even with a consistent delay value, is it OK to repeatedly
> call this function from other layers? Since this is publicly exposed
> from hci_core.h, any code within the subsystem can make calls to this.
> So you should make sure that it doesn't break this, or cover that edge
> case inside this function, if necessary.
So queue_delayed_work when you call it first time just schedule job
and returns 0, if you call it again it will return nozero value if
structure was already waiting in the queue.
I actually depend on this behaviour to make sure I don't restart scan
too often, maximum once every DISCOV_LE_RESTART_DELAY.
>
>>>
>>>> +
>>>> +static void le_scan_restart_work_complete(struct hci_dev *hdev, u8 status)
>>>> +{
>>>> + clear_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags);
>>>> +
>>>> + if (status)
>>>> + BT_ERR("Failed to restart LE scanning: status %d", status);
>>>> +}
>>>> +
>>>> +static void le_scan_restart_work(struct work_struct *work)
>>>> +{
>>>> + struct hci_dev *hdev = container_of(work, struct hci_dev,
>>>> + le_scan_restart.work);
>>>> + struct hci_request req;
>>>> + struct hci_cp_le_set_scan_enable cp;
>>>> + int err;
>>>> +
>>>> + BT_DBG("%s", hdev->name);
>>>> +
>>>> + hci_req_init(&req, hdev);
>>>> +
>>>> + hci_req_add_le_scan_disable(&req);
>>>> +
>>>> + memset(&cp, 0, sizeof(cp));
>>>> + cp.enable = LE_SCAN_ENABLE;
>>>> + hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
>>>> +
>>>> + set_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags);
>>>> +
>>>> + err = hci_req_run(&req, le_scan_restart_work_complete);
>>>> + if (err)
>>>> + BT_ERR("Disable LE scanning request failed: err %d", err);
>>>> +}
>>>> +
>>>> static void set_random_addr(struct hci_request *req, bdaddr_t *rpa)
>>>> {
>>>> struct hci_dev *hdev = req->hdev;
>>>> @@ -4037,6 +4078,7 @@ struct hci_dev *hci_alloc_dev(void)
>>>> INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
>>>> INIT_DELAYED_WORK(&hdev->discov_off, hci_discov_off);
>>>> INIT_DELAYED_WORK(&hdev->le_scan_disable, le_scan_disable_work);
>>>> + INIT_DELAYED_WORK(&hdev->le_scan_restart, le_scan_restart_work);
>>>>
>>>> skb_queue_head_init(&hdev->rx_q);
>>>> skb_queue_head_init(&hdev->cmd_q);
>>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>>> index bd0a801..ed4d5e1 100644
>>>> --- a/net/bluetooth/hci_event.c
>>>> +++ b/net/bluetooth/hci_event.c
>>>> @@ -1157,10 +1157,13 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>>>> d->last_adv_data_len, NULL, 0);
>>>> }
>>>>
>>>> - /* Cancel this timer so that we don't try to disable scanning
>>>> - * when it's already disabled.
>>>> + /* If HCI_LE_SCAN_RESTARTING is set, don't cancel this timer,
>>>> + * because we're just restarting scan. Otherwise cancel it so
>>>> + * that we don't try to disable scanning when it's already
>>>> + * disabled.
>>>> */
>>>> - cancel_delayed_work(&hdev->le_scan_disable);
>>>> + if (!test_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags))
>>>> + cancel_delayed_work(&hdev->le_scan_disable);
>>>>
>>>> clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
>>>>
>>>> --
>>>> 2.1.0.rc2.206.gedb03e5
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>> Thanks,
>>> Arman
>
> Arman
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 2/3] Bluetooth: Extract generic start and stop discovery
2014-11-20 23:40 ` Arman Uguray
@ 2014-11-20 23:55 ` Jakub Pawlowski
0 siblings, 0 replies; 15+ messages in thread
From: Jakub Pawlowski @ 2014-11-20 23:55 UTC (permalink / raw)
To: Arman Uguray; +Cc: BlueZ development
On Thu, Nov 20, 2014 at 3:40 PM, Arman Uguray <armansito@chromium.org> wrote:
> Hi Jakub,
>
>> On Thu, Nov 20, 2014 at 3:14 PM, Jakub Pawlowski <jpawlowski@google.com> wrote:
>> On Thu, Nov 20, 2014 at 2:14 PM, Arman Uguray <armansito@chromium.org> wrote:
>>> Hi Jakub,
>>>
>>>> On Thu, Nov 20, 2014 at 9:51 AM, Jakub Pawlowski <jpawlowski@google.com> wrote:
>>>> This commit extract generic_start_discovery and generic_stop_discovery
>>>> in preparation for start and stop service discovery. The reason behind
>>>> that is that both functions will share big part of code, and it would
>>>> be much easier to maintain just one generic method.
>>>>
>>>> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
>>>> ---
>>>> net/bluetooth/mgmt.c | 147 ++++++++++++++++++++++++++++++---------------------
>>>> 1 file changed, 86 insertions(+), 61 deletions(-)
>>>>
>>>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>>>> index cbeef5f..f650d30 100644
>>>> --- a/net/bluetooth/mgmt.c
>>>> +++ b/net/bluetooth/mgmt.c
>>>> @@ -3675,7 +3675,8 @@ done:
>>>> return err;
>>>> }
>>>>
>>>> -static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status)
>>>> +static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status,
>>>> + uint16_t opcode)
>>>> {
>>>> struct pending_cmd *cmd;
>>>> u8 type;
>>>> @@ -3683,7 +3684,7 @@ static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status)
>>>>
>>>> hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>>>>
>>>> - cmd = mgmt_pending_find(MGMT_OP_START_DISCOVERY, hdev);
>>>> + cmd = mgmt_pending_find(opcode, hdev);
>>>> if (!cmd)
>>>> return -ENOENT;
>>>>
>>>> @@ -3696,7 +3697,8 @@ static int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status)
>>>> return err;
>>>> }
>>>>
>>>> -static void start_discovery_complete(struct hci_dev *hdev, u8 status)
>>>> +static void generic_start_discovery_complete(struct hci_dev *hdev, u8 status,
>>>> + uint16_t opcode)
>>>
>>> I'm not that big on calling these generic functions "generic_*". Let's
>>> try to come up with something better.
>>>
>>>> {
>>>> unsigned long timeout = 0;
>>>>
>>>> @@ -3704,7 +3706,7 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status)
>>>>
>>>> if (status) {
>>>> hci_dev_lock(hdev);
>>>> - mgmt_start_discovery_failed(hdev, status);
>>>> + mgmt_start_discovery_failed(hdev, status, opcode);
>>>> hci_dev_unlock(hdev);
>>>> return;
>>>> }
>>>> @@ -3735,8 +3737,13 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status)
>>>> queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable, timeout);
>>>> }
>>>>
>>>> -static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>>>> - void *data, u16 len)
>>>> +static void start_discovery_complete(struct hci_dev *hdev, u8 status)
>>>> +{
>>>> + generic_start_discovery_complete(hdev, status, MGMT_OP_START_DISCOVERY);
>>>> +}
>>>> +
>>>> +static int generic_start_discovery(struct sock *sk, struct hci_dev *hdev,
>>>> + void *data, u16 len, uint16_t opcode)
>>>> {
>>>> struct mgmt_cp_start_discovery *cp = data;
>>>> struct pending_cmd *cmd;
>>>> @@ -3746,41 +3753,41 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>>>> struct hci_request req;
>>>> /* General inquiry access code (GIAC) */
>>>> u8 lap[3] = { 0x33, 0x8b, 0x9e };
>>>> - u8 status, own_addr_type;
>>>> + u8 status, own_addr_type, type;
>>>> int err;
>>>>
>>>> BT_DBG("%s", hdev->name);
>>>>
>>>> + type = cp->type;
>>>> +
>>>
>>> I don't see the point of this local variable & assignment. Why not
>>> just leave it as it is, "cp->type" isn't that wordy.
>>
>> So this patch is preparation for next patch which will have if/else
>> statement here that would pick proper type for different cases.
>> The next patch is already big so I wanted to make this change from
>> cp->type to type in this one. If that's a bad idea I'll move that to
>> next patch.
>>
>
> Yeah, from here it just looks like a random refactor. I would just put
> it into the next patch. Or if you think that the next patch is too
> big, maybe think about how you can make it smaller by separating the
> logical steps?
Ok, I'll move it to next patch
>
>>>
>>>> hci_dev_lock(hdev);
>>>>
>>>> if (!hdev_is_powered(hdev)) {
>>>> - err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
>>>> - MGMT_STATUS_NOT_POWERED,
>>>> - &cp->type, sizeof(cp->type));
>>>> + err = cmd_complete(sk, hdev->id, opcode,
>>>> + MGMT_STATUS_NOT_POWERED, &type,
>>>> + sizeof(type));
>>>> goto failed;
>>>> }
>>>>
>>>> if (test_bit(HCI_PERIODIC_INQ, &hdev->dev_flags)) {
>>>> - err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
>>>> - MGMT_STATUS_BUSY, &cp->type,
>>>> - sizeof(cp->type));
>>>> + err = cmd_complete(sk, hdev->id, opcode,
>>>> + MGMT_STATUS_BUSY, &type, sizeof(type));
>>>> goto failed;
>>>> }
>>>>
>>>> if (hdev->discovery.state != DISCOVERY_STOPPED) {
>>>> - err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
>>>> - MGMT_STATUS_BUSY, &cp->type,
>>>> - sizeof(cp->type));
>>>> + err = cmd_complete(sk, hdev->id, opcode,
>>>> + MGMT_STATUS_BUSY, &type, sizeof(type));
>>>> goto failed;
>>>> }
>>>>
>>>> - cmd = mgmt_pending_add(sk, MGMT_OP_START_DISCOVERY, hdev, NULL, 0);
>>>> + cmd = mgmt_pending_add(sk, opcode, hdev, NULL, 0);
>>>> if (!cmd) {
>>>> err = -ENOMEM;
>>>> goto failed;
>>>> }
>>>>
>>>> - hdev->discovery.type = cp->type;
>>>> + hdev->discovery.type = type;
>>>>
>>>> hci_req_init(&req, hdev);
>>>>
>>>> @@ -3788,18 +3795,16 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>>>> case DISCOV_TYPE_BREDR:
>>>> status = mgmt_bredr_support(hdev);
>>>> if (status) {
>>>> - err = cmd_complete(sk, hdev->id,
>>>> - MGMT_OP_START_DISCOVERY, status,
>>>> - &cp->type, sizeof(cp->type));
>>>> + err = cmd_complete(sk, hdev->id, opcode, status, &type,
>>>> + sizeof(type));
>>>> mgmt_pending_remove(cmd);
>>>> goto failed;
>>>> }
>>>>
>>>> if (test_bit(HCI_INQUIRY, &hdev->flags)) {
>>>> - err = cmd_complete(sk, hdev->id,
>>>> - MGMT_OP_START_DISCOVERY,
>>>> - MGMT_STATUS_BUSY, &cp->type,
>>>> - sizeof(cp->type));
>>>> + err = cmd_complete(sk, hdev->id, opcode,
>>>> + MGMT_STATUS_BUSY, &type,
>>>> + sizeof(type));
>>>> mgmt_pending_remove(cmd);
>>>> goto failed;
>>>> }
>>>> @@ -3816,19 +3821,17 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>>>> case DISCOV_TYPE_INTERLEAVED:
>>>> status = mgmt_le_support(hdev);
>>>> if (status) {
>>>> - err = cmd_complete(sk, hdev->id,
>>>> - MGMT_OP_START_DISCOVERY, status,
>>>> - &cp->type, sizeof(cp->type));
>>>> + err = cmd_complete(sk, hdev->id, opcode, status, &type,
>>>> + sizeof(type));
>>>> mgmt_pending_remove(cmd);
>>>> goto failed;
>>>> }
>>>>
>>>> if (hdev->discovery.type == DISCOV_TYPE_INTERLEAVED &&
>>>> !test_bit(HCI_BREDR_ENABLED, &hdev->dev_flags)) {
>>>> - err = cmd_complete(sk, hdev->id,
>>>> - MGMT_OP_START_DISCOVERY,
>>>> - MGMT_STATUS_NOT_SUPPORTED,
>>>> - &cp->type, sizeof(cp->type));
>>>> + err = cmd_complete(sk, hdev->id, opcode,
>>>> + MGMT_STATUS_NOT_SUPPORTED, &type,
>>>> + sizeof(type));
>>>> mgmt_pending_remove(cmd);
>>>> goto failed;
>>>> }
>>>> @@ -3840,11 +3843,9 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>>>> */
>>>> if (hci_conn_hash_lookup_state(hdev, LE_LINK,
>>>> BT_CONNECT)) {
>>>> - err = cmd_complete(sk, hdev->id,
>>>> - MGMT_OP_START_DISCOVERY,
>>>> - MGMT_STATUS_REJECTED,
>>>> - &cp->type,
>>>> - sizeof(cp->type));
>>>> + err = cmd_complete(sk, hdev->id, opcode,
>>>> + MGMT_STATUS_REJECTED, &type,
>>>> + sizeof(type));
>>>> mgmt_pending_remove(cmd);
>>>> goto failed;
>>>> }
>>>> @@ -3867,10 +3868,9 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>>>> */
>>>> err = hci_update_random_address(&req, true, &own_addr_type);
>>>> if (err < 0) {
>>>> - err = cmd_complete(sk, hdev->id,
>>>> - MGMT_OP_START_DISCOVERY,
>>>> - MGMT_STATUS_FAILED,
>>>> - &cp->type, sizeof(cp->type));
>>>> + err = cmd_complete(sk, hdev->id, opcode,
>>>> + MGMT_STATUS_FAILED, &type,
>>>> + sizeof(type));
>>>> mgmt_pending_remove(cmd);
>>>> goto failed;
>>>> }
>>>> @@ -3890,14 +3890,15 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>>>> break;
>>>>
>>>> default:
>>>> - err = cmd_complete(sk, hdev->id, MGMT_OP_START_DISCOVERY,
>>>> - MGMT_STATUS_INVALID_PARAMS,
>>>> - &cp->type, sizeof(cp->type));
>>>> + err = cmd_complete(sk, hdev->id, opcode,
>>>> + MGMT_STATUS_INVALID_PARAMS, &type,
>>>> + sizeof(type));
>>>> mgmt_pending_remove(cmd);
>>>> goto failed;
>>>> }
>>>>
>>>> err = hci_req_run(&req, start_discovery_complete);
>>>> +
>>>> if (err < 0)
>>>> mgmt_pending_remove(cmd);
>>>> else
>>>> @@ -3908,12 +3909,20 @@ failed:
>>>> return err;
>>>> }
>>>>
>>>> -static int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status)
>>>> +static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>>>> + void *data, u16 len)
>>>> +{
>>>> + return generic_start_discovery(sk, hdev, data, len,
>>>> + MGMT_OP_START_DISCOVERY);
>>>> +}
>>>> +
>>>> +static int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status,
>>>> + uint16_t opcode)
>>>> {
>>>> struct pending_cmd *cmd;
>>>> int err;
>>>>
>>>> - cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, hdev);
>>>> + cmd = mgmt_pending_find(opcode, hdev);
>>>> if (!cmd)
>>>> return -ENOENT;
>>>>
>>>> @@ -3924,14 +3933,15 @@ static int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status)
>>>> return err;
>>>> }
>>>>
>>>> -static void stop_discovery_complete(struct hci_dev *hdev, u8 status)
>>>> +static void generic_stop_discovery_complete(struct hci_dev *hdev, u8 status,
>>>> + uint16_t opcode)
>>>> {
>>>> BT_DBG("status %d", status);
>>>>
>>>> hci_dev_lock(hdev);
>>>>
>>>> if (status) {
>>>> - mgmt_stop_discovery_failed(hdev, status);
>>>> + mgmt_stop_discovery_failed(hdev, status, opcode);
>>>> goto unlock;
>>>> }
>>>>
>>>> @@ -3941,33 +3951,40 @@ unlock:
>>>> hci_dev_unlock(hdev);
>>>> }
>>>>
>>>> -static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>>>> - u16 len)
>>>> +static void stop_discovery_complete(struct hci_dev *hdev, u8 status)
>>>> +{
>>>> + generic_stop_discovery_complete(hdev, status, MGMT_OP_STOP_DISCOVERY);
>>>> +}
>>>> +
>>>> +static int generic_stop_discovery(struct sock *sk, struct hci_dev *hdev,
>>>> + void *data, u16 len, uint16_t opcode)
>>>> {
>>>> - struct mgmt_cp_stop_discovery *mgmt_cp = data;
>>>> struct pending_cmd *cmd;
>>>> struct hci_request req;
>>>> + struct mgmt_cp_stop_discovery *mgmt_cp = data;
>>>
>>> Why move this?
>>
>> I'll fix that
>>>
>>>> int err;
>>>> + u8 type;
>>>>
>>>
>>> Again, there's no need for this variable.
>>
>> same as aboce - this patch is preparation for next patch which will
>> have if/else statement here that would pick proper type for different
>> cases.
>>
>>>
>>>> BT_DBG("%s", hdev->name);
>>>>
>>>> + type = mgmt_cp->type;
>>>> +
>>>> hci_dev_lock(hdev);
>>>>
>>>> if (!hci_discovery_active(hdev)) {
>>>> - err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY,
>>>> - MGMT_STATUS_REJECTED, &mgmt_cp->type,
>>>> - sizeof(mgmt_cp->type));
>>>> + err = cmd_complete(sk, hdev->id, opcode,
>>>> + MGMT_STATUS_REJECTED, &type, sizeof(type));
>>>> goto unlock;
>>>> }
>>>>
>>>> - if (hdev->discovery.type != mgmt_cp->type) {
>>>> - err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY,
>>>> - MGMT_STATUS_INVALID_PARAMS, &mgmt_cp->type,
>>>> - sizeof(mgmt_cp->type));
>>>> + if (hdev->discovery.type != type) {
>>>> + err = cmd_complete(sk, hdev->id, opcode,
>>>> + MGMT_STATUS_INVALID_PARAMS, &type,
>>>> + sizeof(type));
>>>> goto unlock;
>>>> }
>>>>
>>>> - cmd = mgmt_pending_add(sk, MGMT_OP_STOP_DISCOVERY, hdev, NULL, 0);
>>>> + cmd = mgmt_pending_add(sk, opcode, hdev, NULL, 0);
>>>> if (!cmd) {
>>>> err = -ENOMEM;
>>>> goto unlock;
>>>> @@ -3978,6 +3995,7 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>>>> hci_stop_discovery(&req);
>>>>
>>>> err = hci_req_run(&req, stop_discovery_complete);
>>>> +
>>>> if (!err) {
>>>> hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
>>>> goto unlock;
>>>> @@ -3987,8 +4005,8 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>>>>
>>>> /* If no HCI commands were sent we're done */
>>>> if (err == -ENODATA) {
>>>> - err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY, 0,
>>>> - &mgmt_cp->type, sizeof(mgmt_cp->type));
>>>> + err = cmd_complete(sk, hdev->id, opcode, 0,
>>>> + &type, sizeof(type));
>>>> hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
>>>> }
>>>>
>>>> @@ -3997,6 +4015,13 @@ unlock:
>>>> return err;
>>>> }
>>>>
>>>> +static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>>>> + u16 len)
>>>> +{
>>>> + return generic_stop_discovery(sk, hdev, data, len,
>>>> + MGMT_OP_STOP_DISCOVERY);
>>>> +}
>>>> +
>>>> static int confirm_name(struct sock *sk, struct hci_dev *hdev, void *data,
>>>> u16 len)
>>>> {
>>>> --
>>>> 2.1.0.rc2.206.gedb03e5
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>> Overall I think this patch looks good, except for a few nits that I
>>> commented on above.
>>>
>>> Thanks,
>>> Arman
>
> Thanks,
> Arman
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 1/3] Bluetooth: add hci_restart_le_scan
2014-11-20 23:54 ` Jakub Pawlowski
@ 2014-11-20 23:56 ` Arman Uguray
0 siblings, 0 replies; 15+ messages in thread
From: Arman Uguray @ 2014-11-20 23:56 UTC (permalink / raw)
To: Jakub Pawlowski; +Cc: BlueZ development
Hi Jakub,
> On Thu, Nov 20, 2014 at 3:54 PM, Jakub Pawlowski <jpawlowski@google.com> wrote:
> On Thu, Nov 20, 2014 at 3:37 PM, Arman Uguray <armansito@chromium.org> wrote:
>> Hi Jakub,
>>
>>> On Thu, Nov 20, 2014 at 2:24 PM, Jakub Pawlowski <jpawlowski@google.com> wrote:
>>> On Thu, Nov 20, 2014 at 1:48 PM, Arman Uguray <armansito@chromium.org> wrote:
>>>> Hi Jakub,
>>>>
>>>>> On Thu, Nov 20, 2014 at 9:51 AM, Jakub Pawlowski <jpawlowski@google.com> wrote:
>>>>> Currently there is no way to restart le scan. It's needed in
>>>>> preparation for new service scan method. The way it work: it disable,
>>>>> and then enable le scan on controller. During this restart special flag
>>>>> is set to make sure we won't remove disable scan work from workqueue.
>>>>>
>>>>> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
>>>>> ---
>>>>> include/net/bluetooth/hci.h | 1 +
>>>>> include/net/bluetooth/hci_core.h | 2 ++
>>>>> net/bluetooth/hci_core.c | 42 ++++++++++++++++++++++++++++++++++++++++
>>>>> net/bluetooth/hci_event.c | 9 ++++++---
>>>>> 4 files changed, 51 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>>>>> index e56f909..2f0bce2 100644
>>>>> --- a/include/net/bluetooth/hci.h
>>>>> +++ b/include/net/bluetooth/hci.h
>>>>> @@ -203,6 +203,7 @@ enum {
>>>>> HCI_FAST_CONNECTABLE,
>>>>> HCI_BREDR_ENABLED,
>>>>> HCI_LE_SCAN_INTERRUPTED,
>>>>> + HCI_LE_SCAN_RESTARTING,
>>>>> };
>>>>>
>>>>> /* A mask for the flags that are supposed to remain when a reset happens
>>>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>>>> index 396c098..36211f9 100644
>>>>> --- a/include/net/bluetooth/hci_core.h
>>>>> +++ b/include/net/bluetooth/hci_core.h
>>>>> @@ -337,6 +337,7 @@ struct hci_dev {
>>>>> unsigned long dev_flags;
>>>>>
>>>>> struct delayed_work le_scan_disable;
>>>>> + struct delayed_work le_scan_restart;
>>>>>
>>>>> __s8 adv_tx_power;
>>>>> __u8 adv_data[HCI_MAX_AD_LENGTH];
>>>>> @@ -1403,6 +1404,7 @@ int hci_update_random_address(struct hci_request *req, bool require_privacy,
>>>>> u8 *own_addr_type);
>>>>> void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
>>>>> u8 *bdaddr_type);
>>>>> +void hci_restart_le_scan(struct hci_dev *hdev, unsigned long delay);
>>>>>
>>>>> #define SCO_AIRMODE_MASK 0x0003
>>>>> #define SCO_AIRMODE_CVSD 0x0000
>>>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>>>> index 5c319a4..858cf54 100644
>>>>> --- a/net/bluetooth/hci_core.c
>>>>> +++ b/net/bluetooth/hci_core.c
>>>>> @@ -2557,6 +2557,7 @@ static int hci_dev_do_close(struct hci_dev *hdev)
>>>>> cancel_delayed_work(&hdev->service_cache);
>>>>>
>>>>> cancel_delayed_work_sync(&hdev->le_scan_disable);
>>>>> + cancel_delayed_work_sync(&hdev->le_scan_restart);
>>>>>
>>>>> if (test_bit(HCI_MGMT, &hdev->dev_flags))
>>>>> cancel_delayed_work_sync(&hdev->rpa_expired);
>>>>> @@ -3851,6 +3852,8 @@ static void le_scan_disable_work(struct work_struct *work)
>>>>>
>>>>> BT_DBG("%s", hdev->name);
>>>>>
>>>>> + cancel_delayed_work_sync(&hdev->le_scan_restart);
>>>>> +
>>>>> hci_req_init(&req, hdev);
>>>>>
>>>>> hci_req_add_le_scan_disable(&req);
>>>>> @@ -3860,6 +3863,44 @@ static void le_scan_disable_work(struct work_struct *work)
>>>>> BT_ERR("Disable LE scanning request failed: err %d", err);
>>>>> }
>>>>>
>>>>> +void hci_restart_le_scan(struct hci_dev *hdev, unsigned long delay)
>>>>> +{
>>>>> + queue_delayed_work(hdev->workqueue, &hdev->le_scan_restart, delay);
>>>>> +}
>>>>
>>>> Since you're exposing this as a public API, does it make sense to
>>>> check here if a delayed work has already been scheduled? I.e. is it OK
>>>> if this function gets called several times in succession with
>>>> different delay values?
>>>
>>> Next patch introduces DISCOV_LE_RESTART_DELAY msecs_to_jiffies(300)
>>> which is the only delay value used to schedule le_scan_restart right now.
>>> I might change the implementation to:
>>>
>>> void hci_restart_le_scan(struct hci_dev *hdev)
>>> {
>>> queue_delayed_work(hdev->workqueue, &hdev->le_scan_restart,
>>> DISCOV_LE_RESTART_DELAY);
>>> }
>>>
>>
>> Sure but even with a consistent delay value, is it OK to repeatedly
>> call this function from other layers? Since this is publicly exposed
>> from hci_core.h, any code within the subsystem can make calls to this.
>> So you should make sure that it doesn't break this, or cover that edge
>> case inside this function, if necessary.
>
> So queue_delayed_work when you call it first time just schedule job
> and returns 0, if you call it again it will return nozero value if
> structure was already waiting in the queue.
> I actually depend on this behaviour to make sure I don't restart scan
> too often, maximum once every DISCOV_LE_RESTART_DELAY.
>
Cool, that's what I wanted to confirm. In that case I'm OK with this.
>>
>>>>
>>>>> +
>>>>> +static void le_scan_restart_work_complete(struct hci_dev *hdev, u8 status)
>>>>> +{
>>>>> + clear_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags);
>>>>> +
>>>>> + if (status)
>>>>> + BT_ERR("Failed to restart LE scanning: status %d", status);
>>>>> +}
>>>>> +
>>>>> +static void le_scan_restart_work(struct work_struct *work)
>>>>> +{
>>>>> + struct hci_dev *hdev = container_of(work, struct hci_dev,
>>>>> + le_scan_restart.work);
>>>>> + struct hci_request req;
>>>>> + struct hci_cp_le_set_scan_enable cp;
>>>>> + int err;
>>>>> +
>>>>> + BT_DBG("%s", hdev->name);
>>>>> +
>>>>> + hci_req_init(&req, hdev);
>>>>> +
>>>>> + hci_req_add_le_scan_disable(&req);
>>>>> +
>>>>> + memset(&cp, 0, sizeof(cp));
>>>>> + cp.enable = LE_SCAN_ENABLE;
>>>>> + hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
>>>>> +
>>>>> + set_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags);
>>>>> +
>>>>> + err = hci_req_run(&req, le_scan_restart_work_complete);
>>>>> + if (err)
>>>>> + BT_ERR("Disable LE scanning request failed: err %d", err);
>>>>> +}
>>>>> +
>>>>> static void set_random_addr(struct hci_request *req, bdaddr_t *rpa)
>>>>> {
>>>>> struct hci_dev *hdev = req->hdev;
>>>>> @@ -4037,6 +4078,7 @@ struct hci_dev *hci_alloc_dev(void)
>>>>> INIT_DELAYED_WORK(&hdev->power_off, hci_power_off);
>>>>> INIT_DELAYED_WORK(&hdev->discov_off, hci_discov_off);
>>>>> INIT_DELAYED_WORK(&hdev->le_scan_disable, le_scan_disable_work);
>>>>> + INIT_DELAYED_WORK(&hdev->le_scan_restart, le_scan_restart_work);
>>>>>
>>>>> skb_queue_head_init(&hdev->rx_q);
>>>>> skb_queue_head_init(&hdev->cmd_q);
>>>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>>>> index bd0a801..ed4d5e1 100644
>>>>> --- a/net/bluetooth/hci_event.c
>>>>> +++ b/net/bluetooth/hci_event.c
>>>>> @@ -1157,10 +1157,13 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>>>>> d->last_adv_data_len, NULL, 0);
>>>>> }
>>>>>
>>>>> - /* Cancel this timer so that we don't try to disable scanning
>>>>> - * when it's already disabled.
>>>>> + /* If HCI_LE_SCAN_RESTARTING is set, don't cancel this timer,
>>>>> + * because we're just restarting scan. Otherwise cancel it so
>>>>> + * that we don't try to disable scanning when it's already
>>>>> + * disabled.
>>>>> */
>>>>> - cancel_delayed_work(&hdev->le_scan_disable);
>>>>> + if (!test_bit(HCI_LE_SCAN_RESTARTING, &hdev->dev_flags))
>>>>> + cancel_delayed_work(&hdev->le_scan_disable);
>>>>>
>>>>> clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
>>>>>
>>>>> --
>>>>> 2.1.0.rc2.206.gedb03e5
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>> Thanks,
>>>> Arman
>>
>> Arman
Arman
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 3/3] Bluetooth: start and stop service discovery
2014-11-20 23:33 ` Arman Uguray
@ 2014-11-21 17:57 ` Jakub Pawlowski
0 siblings, 0 replies; 15+ messages in thread
From: Jakub Pawlowski @ 2014-11-21 17:57 UTC (permalink / raw)
To: Arman Uguray; +Cc: BlueZ development
On Thu, Nov 20, 2014 at 3:33 PM, Arman Uguray <armansito@chromium.org> wrote:
> Hi Jakub,
>
>> On Thu, Nov 20, 2014 at 9:56 AM, Jakub Pawlowski <jpawlowski@google.com> wrote:
>> This patch introduces start service discovery and stop service
>> discovery methods. The reason behind that is to enable users to find
>> specific services in range by UUID. Whole filtering is done in
>> mgmt_device_found.
>>
>> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
>> ---
>> include/net/bluetooth/hci.h | 1 +
>> include/net/bluetooth/hci_core.h | 11 ++
>> include/net/bluetooth/mgmt.h | 24 ++++
>> net/bluetooth/hci_core.c | 1 +
>> net/bluetooth/mgmt.c | 296 +++++++++++++++++++++++++++++++++++++--
>> 5 files changed, 324 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>> index 2f0bce2..97b0893 100644
>> --- a/include/net/bluetooth/hci.h
>> +++ b/include/net/bluetooth/hci.h
>> @@ -204,6 +204,7 @@ enum {
>> HCI_BREDR_ENABLED,
>> HCI_LE_SCAN_INTERRUPTED,
>> HCI_LE_SCAN_RESTARTING,
>> + HCI_SERVICE_FILTER,
>
> I'm not sure if this really belongs in this enum. I don't think
> HCI_SERVICE_FILTER really respresents "a state from the controller".
> So perhaps this should be part of the QUIRK enum instead? Maybe Johan
> should chime in on this.
>
So I was asked to put HCI_SERVICE_FILTER into hdev->dev_flags, and I
looked through the code, and it looks like hdev->dev_flags uses this
enum so I just added my thing there.
>> };
>>
>> /* A mask for the flags that are supposed to remain when a reset happens
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index 36211f9..39009f2 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -68,6 +68,7 @@ struct discovery_state {
>> struct list_head all; /* All devices found during inquiry */
>> struct list_head unknown; /* Name state not known */
>> struct list_head resolve; /* Name needs to be resolved */
>> + struct list_head uuid_filter;
>> __u32 timestamp;
>> bdaddr_t last_adv_addr;
>> u8 last_adv_addr_type;
>> @@ -146,6 +147,12 @@ struct oob_data {
>> u8 rand256[16];
>> };
>>
>> +struct uuid_filter {
>> + struct list_head list;
>> + s8 rssi;
>> + u8 uuid[16];
>> +};
>> +
>> #define HCI_MAX_SHORT_NAME_LENGTH 10
>>
>> /* Default LE RPA expiry time, 15 minutes */
>> @@ -502,6 +509,7 @@ static inline void discovery_init(struct hci_dev *hdev)
>> INIT_LIST_HEAD(&hdev->discovery.all);
>> INIT_LIST_HEAD(&hdev->discovery.unknown);
>> INIT_LIST_HEAD(&hdev->discovery.resolve);
>> + INIT_LIST_HEAD(&hdev->discovery.uuid_filter);
>> }
>>
>> bool hci_discovery_active(struct hci_dev *hdev);
>> @@ -1328,6 +1336,8 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event);
>> #define DISCOV_INTERLEAVED_INQUIRY_LEN 0x04
>> #define DISCOV_BREDR_INQUIRY_LEN 0x08
>>
>> +#define DISCOV_LE_RESTART_DELAY msecs_to_jiffies(300)
>> +
>> int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
>> int mgmt_new_settings(struct hci_dev *hdev);
>> void mgmt_index_added(struct hci_dev *hdev);
>> @@ -1394,6 +1404,7 @@ void mgmt_new_conn_param(struct hci_dev *hdev, bdaddr_t *bdaddr,
>> u16 max_interval, u16 latency, u16 timeout);
>> void mgmt_reenable_advertising(struct hci_dev *hdev);
>> void mgmt_smp_complete(struct hci_conn *conn, bool complete);
>> +void mgmt_clean_up_service_discovery(struct hci_dev *hdev);
>>
>> u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
>> u16 to_multiplier);
>> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
>> index b391fd6..202caf7 100644
>> --- a/include/net/bluetooth/mgmt.h
>> +++ b/include/net/bluetooth/mgmt.h
>> @@ -495,6 +495,30 @@ struct mgmt_cp_set_public_address {
>> } __packed;
>> #define MGMT_SET_PUBLIC_ADDRESS_SIZE 6
>>
>> +#define MGMT_OP_START_SERVICE_DISCOVERY 0x003A
>
> I think I commented before that I don't like that this command is
> called "Service Discovery" which makes me think of SDP or even GATT
> instead of a "device scan filtered by service UUID". So I don't know
> if it's too late to change the name of the command, I guess Marcel
> needs to be in that fight too. Something like "Start Filtered Device
> Scan" would be much more intuitive IMO, since we have proximity
> filters here as well.
>
I think that marcel was having some concerns about naming generic*
methods I introducet in mgmt.c, but didn't say anything about this
one. Actually I think that I took this name from his patch to
doc/mgmt-api.txt so it should be ok.
http://marc.info/?l=linux-bluetooth&m=141293816613415&w=2
>> +#define MGMT_START_SERVICE_DISCOVERY_SIZE 1
>> +
>> +#define MGMT_RANGE_NONE 0x00
>> +#define MGMT_RANGE_RSSI 0x01
>> +#define MGMT_RANGE_PATHLOSS 0x02
>> +
>
> Do these represent the different proximity filters? Then we should
> name this accordingly; MGMT_RANGE_* sounds to generic to me, maybe
> MGMT_PROXIMITY_FILTER_* (or something like that)?
>
> Also, what is the *_PATHLOSS macro for? Aren't we always filtering by
> RSSI? Actually, why aren't we always filtering by pathloss? Eitherway,
> if PATHLOSS is unused I would just remove it for now.
>
All MGMT_RANGE_* are no longer used, I forgot to remove that.
>> +struct mgmt_uuid_filter {
>> + __s8 rssi;
>> + __u8 uuid[16];
>> +} __packed;
>> +
>> +struct mgmt_cp_start_service_discovery {
>> + __u8 type;
>> + __le16 filter_count;
>> + struct mgmt_uuid_filter filter[0];
>> +} __packed;
>> +
>> +#define MGMT_OP_STOP_SERVICE_DISCOVERY 0x003B
>> +struct mgmt_cp_stop_service_discovery {
>> + __u8 type;
>> +} __packed;
>> +#define MGMT_STOP_SERVICE_DISCOVERY_SIZE 1
>> +
>> #define MGMT_EV_CMD_COMPLETE 0x0001
>> struct mgmt_ev_cmd_complete {
>> __le16 opcode;
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 858cf54..fab6755 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -3853,6 +3853,7 @@ static void le_scan_disable_work(struct work_struct *work)
>> BT_DBG("%s", hdev->name);
>>
>> cancel_delayed_work_sync(&hdev->le_scan_restart);
>> + mgmt_clean_up_service_discovery(hdev);
>>
>> hci_req_init(&req, hdev);
>>
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index f650d30..ed67060 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -93,6 +93,8 @@ static const u16 mgmt_commands[] = {
>> MGMT_OP_READ_CONFIG_INFO,
>> MGMT_OP_SET_EXTERNAL_CONFIG,
>> MGMT_OP_SET_PUBLIC_ADDRESS,
>> + MGMT_OP_START_SERVICE_DISCOVERY,
>> + MGMT_OP_STOP_SERVICE_DISCOVERY,
>> };
>>
>> static const u16 mgmt_events[] = {
>> @@ -1258,6 +1260,23 @@ static void clean_up_hci_complete(struct hci_dev *hdev, u8 status)
>> }
>> }
>>
>> +void mgmt_clean_up_service_discovery(struct hci_dev *hdev)
>> +{
>
> Can you perhaps add a comment somewhere that this function in effect
> cleans up the state set up by the "Start Service Discovery" function.
> It's not easy to make that association just by looking at the function
> name while we have a dozen functions with the name "discovery" in it
> that correspond to the other mgmt device discovery command.
>
>> + struct uuid_filter *filter;
>> + struct uuid_filter *tmp;
>> +
>> + if (!test_and_clear_bit(HCI_SERVICE_FILTER, &hdev->dev_flags))
>> + return;
>> +
>> + cancel_delayed_work_sync(&hdev->le_scan_restart);
>> +
>> + list_for_each_entry_safe(filter, tmp, &hdev->discovery.uuid_filter,
>> + list) {
>> + __list_del_entry(&filter->list);
>> + kfree(filter);
>> + }
>> +}
>> +
>> static bool hci_stop_discovery(struct hci_request *req)
>> {
>> struct hci_dev *hdev = req->hdev;
>> @@ -1270,6 +1289,7 @@ static bool hci_stop_discovery(struct hci_request *req)
>> hci_req_add(req, HCI_OP_INQUIRY_CANCEL, 0, NULL);
>> } else {
>> cancel_delayed_work(&hdev->le_scan_disable);
>> + mgmt_clean_up_service_discovery(hdev);
>> hci_req_add_le_scan_disable(req);
>> }
>>
>> @@ -3742,23 +3762,75 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status)
>> generic_start_discovery_complete(hdev, status, MGMT_OP_START_DISCOVERY);
>> }
>>
>> +static void start_service_discovery_complete(struct hci_dev *hdev, u8 status)
>> +{
>> + generic_start_discovery_complete(hdev, status,
>> + MGMT_OP_START_SERVICE_DISCOVERY);
>> +}
>> +
>> +static int init_service_discovery(struct hci_dev *hdev,
>> + struct mgmt_uuid_filter *filters,
>> + u16 filter_cnt)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < filter_cnt; i++) {
>> + struct mgmt_uuid_filter *key = &filters[i];
>> + struct uuid_filter *tmp;
>> +
>> + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
>> + if (!tmp)
>> + return -ENOMEM;
>> +
>> + memcpy(tmp->uuid, key->uuid, 16);
>> + tmp->rssi = key->rssi;
>> + INIT_LIST_HEAD(&tmp->list);
>> +
>> + list_add(&tmp->list, &hdev->discovery.uuid_filter);
>> + }
>> + set_bit(HCI_SERVICE_FILTER, &hdev->dev_flags);
>> + return 0;
>> +}
>> +
>> static int generic_start_discovery(struct sock *sk, struct hci_dev *hdev,
>> void *data, u16 len, uint16_t opcode)
>> {
>> - struct mgmt_cp_start_discovery *cp = data;
>> struct pending_cmd *cmd;
>> struct hci_cp_le_set_scan_param param_cp;
>> struct hci_cp_le_set_scan_enable enable_cp;
>> struct hci_cp_inquiry inq_cp;
>> struct hci_request req;
>> + struct mgmt_uuid_filter *serv_filters = NULL;
>> /* General inquiry access code (GIAC) */
>> u8 lap[3] = { 0x33, 0x8b, 0x9e };
>> u8 status, own_addr_type, type;
>> int err;
>> + u16 serv_filter_cnt = 0;
>>
>> BT_DBG("%s", hdev->name);
>>
>> - type = cp->type;
>> + if (opcode == MGMT_OP_START_SERVICE_DISCOVERY) {
>> + struct mgmt_cp_start_service_discovery *cp = data;
>> + u16 expected_len, filter_count;
>> +
>> + type = cp->type;
>> + filter_count = __le16_to_cpu(cp->filter_count);
>> + expected_len = sizeof(*cp) +
>> + filter_count * sizeof(struct mgmt_uuid_filter);
>> +
>> + if (expected_len != len) {
>> + return cmd_complete(sk, hdev->id, opcode,
>> + MGMT_STATUS_INVALID_PARAMS, &type,
>> + sizeof(type));
>> + }
>> +
>> + serv_filters = cp->filter;
>> + serv_filter_cnt = filter_count;
>> + } else {
>> + struct mgmt_cp_start_discovery *cp = data;
>> +
>> + type = cp->type;
>> + }
>>
>> hci_dev_lock(hdev);
>>
>> @@ -3897,7 +3969,17 @@ static int generic_start_discovery(struct sock *sk, struct hci_dev *hdev,
>> goto failed;
>> }
>>
>> - err = hci_req_run(&req, start_discovery_complete);
>> + if (serv_filters != NULL) {
>> + err = init_service_discovery(hdev, serv_filters,
>> + serv_filter_cnt);
>> + if (err)
>> + goto failed;
>> + }
>> +
>> + if (opcode == MGMT_OP_START_SERVICE_DISCOVERY)
>> + err = hci_req_run(&req, start_service_discovery_complete);
>> + else
>> + err = hci_req_run(&req, start_discovery_complete);
>>
>> if (err < 0)
>> mgmt_pending_remove(cmd);
>> @@ -3956,18 +4038,31 @@ static void stop_discovery_complete(struct hci_dev *hdev, u8 status)
>> generic_stop_discovery_complete(hdev, status, MGMT_OP_STOP_DISCOVERY);
>> }
>>
>> +static void stop_service_discovery_complete(struct hci_dev *hdev, u8 status)
>> +{
>> + generic_stop_discovery_complete(hdev, status,
>> + MGMT_OP_STOP_SERVICE_DISCOVERY);
>> +}
>> +
>> static int generic_stop_discovery(struct sock *sk, struct hci_dev *hdev,
>> void *data, u16 len, uint16_t opcode)
>> {
>> struct pending_cmd *cmd;
>> struct hci_request req;
>> - struct mgmt_cp_stop_discovery *mgmt_cp = data;
>> int err;
>> u8 type;
>>
>> BT_DBG("%s", hdev->name);
>>
>> - type = mgmt_cp->type;
>> + if (opcode == MGMT_OP_STOP_SERVICE_DISCOVERY) {
>> + struct mgmt_cp_stop_service_discovery *mgmt_cp = data;
>> +
>> + type = mgmt_cp->type;
>> + } else {
>> + struct mgmt_cp_stop_discovery *mgmt_cp = data;
>> +
>> + type = mgmt_cp->type;
>> + }
>>
>> hci_dev_lock(hdev);
>>
>> @@ -3994,7 +4089,10 @@ static int generic_stop_discovery(struct sock *sk, struct hci_dev *hdev,
>>
>> hci_stop_discovery(&req);
>>
>> - err = hci_req_run(&req, stop_discovery_complete);
>> + if (opcode == MGMT_OP_STOP_SERVICE_DISCOVERY)
>> + err = hci_req_run(&req, stop_service_discovery_complete);
>> + else
>> + err = hci_req_run(&req, stop_discovery_complete);
>>
>> if (!err) {
>> hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
>> @@ -5707,6 +5805,20 @@ unlock:
>> return err;
>> }
>>
>> +static int start_service_discovery(struct sock *sk, struct hci_dev *hdev,
>> + void *data, u16 len)
>> +{
>> + return generic_start_discovery(sk, hdev, data, len,
>> + MGMT_OP_START_SERVICE_DISCOVERY);
>> +}
>> +
>> +static int stop_service_discovery(struct sock *sk, struct hci_dev *hdev,
>> + void *data, u16 len)
>> +{
>> + return generic_stop_discovery(sk, hdev, data, len,
>> + MGMT_OP_STOP_SERVICE_DISCOVERY);
>> +}
>> +
>> static const struct mgmt_handler {
>> int (*func) (struct sock *sk, struct hci_dev *hdev, void *data,
>> u16 data_len);
>> @@ -5771,6 +5883,8 @@ static const struct mgmt_handler {
>> { read_config_info, false, MGMT_READ_CONFIG_INFO_SIZE },
>> { set_external_config, false, MGMT_SET_EXTERNAL_CONFIG_SIZE },
>> { set_public_address, false, MGMT_SET_PUBLIC_ADDRESS_SIZE },
>> + { start_service_discovery, true, MGMT_START_SERVICE_DISCOVERY_SIZE },
>> + { stop_service_discovery, false, MGMT_STOP_SERVICE_DISCOVERY_SIZE },
>> };
>>
>> int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
>> @@ -6837,6 +6951,135 @@ void mgmt_read_local_oob_data_complete(struct hci_dev *hdev, u8 *hash192,
>> mgmt_pending_remove(cmd);
>> }
>>
>> +struct parsed_uuid {
>> + struct list_head list;
>> + u8 uuid[16];
>> +};
>> +
>> +/* this is reversed hex representation of bluetooth base uuid. We need it for
>> + * service uuid parsing in eir.
>> + */
>> +static const u8 reverse_base_uuid[] = {
>> + 0xfb, 0x34, 0x9b, 0x5f, 0x80, 0x00, 0x00, 0x80,
>> + 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>> +};
>> +
>> +static int add_uuid_to_list(struct list_head *uuids, u8 *uuid)
>> +{
>> + struct parsed_uuid *tmp_uuid;
>> +
>> + tmp_uuid = kmalloc(sizeof(*tmp_uuid), GFP_KERNEL);
>> + if (tmp_uuid == NULL)
>> + return -ENOMEM;
>> +
>> + memcpy(tmp_uuid->uuid, uuid, 16);
>> + INIT_LIST_HEAD(&tmp_uuid->list);
>> + list_add(&tmp_uuid->list, uuids);
>> + return 0;
>> +}
>> +
>> +static void free_uuids_list(struct list_head *uuids)
>> +{
>> + struct parsed_uuid *uuid, *tmp;
>> +
>> + list_for_each_entry_safe(uuid, tmp, uuids, list) {
>> + __list_del_entry(&uuid->list);
>> + kfree(uuid);
>> + }
>> +}
>> +
>> +static int eir_parse(u8 *eir, u8 eir_len, struct list_head *uuids)
>> +{
>> + size_t offset;
>> + u8 uuid[16];
>> + int i, ret;
>> +
>> + offset = 0;
>> + while (offset < eir_len) {
>> + uint8_t field_len = eir[0];
>> +
>> + /* Check for the end of EIR */
>> + if (field_len == 0)
>> + break;
>> +
>> + if (offset + field_len > eir_len)
>> + return -EINVAL;
>> +
>> + switch (eir[1]) {
>> + case EIR_UUID16_ALL:
>> + case EIR_UUID16_SOME:
>> + for (i = 0; i + 3 <= field_len; i += 2) {
>> + memcpy(uuid, reverse_base_uuid, 16);
>> + uuid[13] = eir[i + 3];
>> + uuid[12] = eir[i + 2];
>> + ret = add_uuid_to_list(uuids, uuid);
>> + if (ret)
>> + return ret;
>> + }
>> + break;
>> + case EIR_UUID32_ALL:
>> + case EIR_UUID32_SOME:
>> + for (i = 0; i + 5 <= field_len; i += 4) {
>> + memcpy(uuid, reverse_base_uuid, 16);
>> + uuid[15] = eir[i + 5];
>> + uuid[14] = eir[i + 4];
>> + uuid[13] = eir[i + 3];
>> + uuid[12] = eir[i + 2];
>> + ret = add_uuid_to_list(uuids, uuid);
>> + if (ret)
>> + return ret;
>> + }
>> + break;
>> + case EIR_UUID128_ALL:
>> + case EIR_UUID128_SOME:
>> + for (i = 0; i + 17 <= field_len; i += 16) {
>> + memcpy(uuid, eir + i + 2, 16);
>> + ret = add_uuid_to_list(uuids, uuid);
>> + if (ret)
>> + return ret;
>> + }
>> + break;
>> + }
>> +
>> + offset += field_len + 1;
>> + eir += field_len + 1;
>> + }
>> + return 0;
>> +}
>> +
>> +enum {
>> + NO_MATCH,
>> + SERVICE_MATCH,
>> + FULL_MATCH
>> +};
>> +
>> +static u8 find_match(struct uuid_filter *filter, u8 uuid[16], s8 rssi)
>> +{
>> + if (memcmp(filter->uuid, uuid, 16) != 0)
>> + return NO_MATCH;
>> + if (rssi >= filter->rssi)
>> + return FULL_MATCH;
>> +
>> + return SERVICE_MATCH;
>> +}
>> +
>> +static u8 find_matches(struct hci_dev *hdev, s8 rssi, struct list_head *uuids)
>> +{
>> + struct uuid_filter *filter;
>> + struct parsed_uuid *uuidptr, *tmp_uuid;
>> + int match_type = NO_MATCH, tmp;
>> +
>> + list_for_each_entry_safe(uuidptr, tmp_uuid, uuids, list) {
>> + list_for_each_entry(filter, &hdev->discovery.uuid_filter,
>> + list) {
>> + tmp = find_match(filter, uuidptr->uuid, rssi);
>> + if (tmp > match_type)
>> + match_type = tmp;
>> + }
>> + }
>> + return match_type;
>> +}
>> +
>> void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>> u8 addr_type, u8 *dev_class, s8 rssi, u32 flags,
>> u8 *eir, u16 eir_len, u8 *scan_rsp, u8 scan_rsp_len)
>> @@ -6844,6 +7087,9 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>> char buf[512];
>> struct mgmt_ev_device_found *ev = (void *) buf;
>> size_t ev_size;
>> + LIST_HEAD(uuids);
>> + int err = 0;
>> + u8 match_type;
>>
>> /* Don't send events for a non-kernel initiated discovery. With
>> * LE one exception is if we have pend_le_reports > 0 in which
>> @@ -6882,7 +7128,31 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>> ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
>> ev_size = sizeof(*ev) + eir_len + scan_rsp_len;
>>
>> - mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
>> + if (!test_bit(HCI_SERVICE_FILTER, &hdev->dev_flags)) {
>> + mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
>> + return;
>> + }
>> +
>> + err = eir_parse(eir, eir_len, &uuids);
>> + if (err) {
>> + free_uuids_list(&uuids);
>> + return;
>> + }
>> +
>> + match_type = find_matches(hdev, rssi, &uuids);
>> + free_uuids_list(&uuids);
>> +
>> + if (match_type == NO_MATCH)
>> + return;
>> +
>> + if (test_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks)) {
>> + queue_delayed_work(hdev->workqueue,
>> + &hdev->le_scan_restart,
>> + DISCOV_LE_RESTART_DELAY);
>> + }
>
> You don't need the curly braces for the above if statement.
>
>> + if (match_type == FULL_MATCH)
>> + mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev,
>> + ev_size, NULL);
>> }
>>
>> void mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
>> @@ -6915,10 +7185,18 @@ void mgmt_discovering(struct hci_dev *hdev, u8 discovering)
>>
>> BT_DBG("%s discovering %u", hdev->name, discovering);
>>
>> - if (discovering)
>> + if (discovering) {
>> cmd = mgmt_pending_find(MGMT_OP_START_DISCOVERY, hdev);
>> - else
>> + if (cmd == NULL)
>> + cmd = mgmt_pending_find(MGMT_OP_START_SERVICE_DISCOVERY,
>> + hdev);
>> +
>> + } else {
>> cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, hdev);
>> + if (cmd == NULL)
>> + cmd = mgmt_pending_find(MGMT_OP_STOP_SERVICE_DISCOVERY,
>> + hdev);
>> + }
>>
>> if (cmd != NULL) {
>> u8 type = hdev->discovery.type;
>> --
>> 2.1.0.rc2.206.gedb03e5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> Thanks,
> Arman
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v7 3/3] Bluetooth: start and stop service discovery
2014-11-20 17:56 [PATCH v7 3/3] Bluetooth: start and stop service discovery Jakub Pawlowski
@ 2014-11-20 23:33 ` Arman Uguray
2014-11-21 17:57 ` Jakub Pawlowski
0 siblings, 1 reply; 15+ messages in thread
From: Arman Uguray @ 2014-11-20 23:33 UTC (permalink / raw)
To: Jakub Pawlowski; +Cc: BlueZ development
Hi Jakub,
> On Thu, Nov 20, 2014 at 9:56 AM, Jakub Pawlowski <jpawlowski@google.com> wrote:
> This patch introduces start service discovery and stop service
> discovery methods. The reason behind that is to enable users to find
> specific services in range by UUID. Whole filtering is done in
> mgmt_device_found.
>
> Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
> ---
> include/net/bluetooth/hci.h | 1 +
> include/net/bluetooth/hci_core.h | 11 ++
> include/net/bluetooth/mgmt.h | 24 ++++
> net/bluetooth/hci_core.c | 1 +
> net/bluetooth/mgmt.c | 296 +++++++++++++++++++++++++++++++++++++--
> 5 files changed, 324 insertions(+), 9 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 2f0bce2..97b0893 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -204,6 +204,7 @@ enum {
> HCI_BREDR_ENABLED,
> HCI_LE_SCAN_INTERRUPTED,
> HCI_LE_SCAN_RESTARTING,
> + HCI_SERVICE_FILTER,
I'm not sure if this really belongs in this enum. I don't think
HCI_SERVICE_FILTER really respresents "a state from the controller".
So perhaps this should be part of the QUIRK enum instead? Maybe Johan
should chime in on this.
> };
>
> /* A mask for the flags that are supposed to remain when a reset happens
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 36211f9..39009f2 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -68,6 +68,7 @@ struct discovery_state {
> struct list_head all; /* All devices found during inquiry */
> struct list_head unknown; /* Name state not known */
> struct list_head resolve; /* Name needs to be resolved */
> + struct list_head uuid_filter;
> __u32 timestamp;
> bdaddr_t last_adv_addr;
> u8 last_adv_addr_type;
> @@ -146,6 +147,12 @@ struct oob_data {
> u8 rand256[16];
> };
>
> +struct uuid_filter {
> + struct list_head list;
> + s8 rssi;
> + u8 uuid[16];
> +};
> +
> #define HCI_MAX_SHORT_NAME_LENGTH 10
>
> /* Default LE RPA expiry time, 15 minutes */
> @@ -502,6 +509,7 @@ static inline void discovery_init(struct hci_dev *hdev)
> INIT_LIST_HEAD(&hdev->discovery.all);
> INIT_LIST_HEAD(&hdev->discovery.unknown);
> INIT_LIST_HEAD(&hdev->discovery.resolve);
> + INIT_LIST_HEAD(&hdev->discovery.uuid_filter);
> }
>
> bool hci_discovery_active(struct hci_dev *hdev);
> @@ -1328,6 +1336,8 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event);
> #define DISCOV_INTERLEAVED_INQUIRY_LEN 0x04
> #define DISCOV_BREDR_INQUIRY_LEN 0x08
>
> +#define DISCOV_LE_RESTART_DELAY msecs_to_jiffies(300)
> +
> int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
> int mgmt_new_settings(struct hci_dev *hdev);
> void mgmt_index_added(struct hci_dev *hdev);
> @@ -1394,6 +1404,7 @@ void mgmt_new_conn_param(struct hci_dev *hdev, bdaddr_t *bdaddr,
> u16 max_interval, u16 latency, u16 timeout);
> void mgmt_reenable_advertising(struct hci_dev *hdev);
> void mgmt_smp_complete(struct hci_conn *conn, bool complete);
> +void mgmt_clean_up_service_discovery(struct hci_dev *hdev);
>
> u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
> u16 to_multiplier);
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index b391fd6..202caf7 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -495,6 +495,30 @@ struct mgmt_cp_set_public_address {
> } __packed;
> #define MGMT_SET_PUBLIC_ADDRESS_SIZE 6
>
> +#define MGMT_OP_START_SERVICE_DISCOVERY 0x003A
I think I commented before that I don't like that this command is
called "Service Discovery" which makes me think of SDP or even GATT
instead of a "device scan filtered by service UUID". So I don't know
if it's too late to change the name of the command, I guess Marcel
needs to be in that fight too. Something like "Start Filtered Device
Scan" would be much more intuitive IMO, since we have proximity
filters here as well.
> +#define MGMT_START_SERVICE_DISCOVERY_SIZE 1
> +
> +#define MGMT_RANGE_NONE 0x00
> +#define MGMT_RANGE_RSSI 0x01
> +#define MGMT_RANGE_PATHLOSS 0x02
> +
Do these represent the different proximity filters? Then we should
name this accordingly; MGMT_RANGE_* sounds to generic to me, maybe
MGMT_PROXIMITY_FILTER_* (or something like that)?
Also, what is the *_PATHLOSS macro for? Aren't we always filtering by
RSSI? Actually, why aren't we always filtering by pathloss? Eitherway,
if PATHLOSS is unused I would just remove it for now.
> +struct mgmt_uuid_filter {
> + __s8 rssi;
> + __u8 uuid[16];
> +} __packed;
> +
> +struct mgmt_cp_start_service_discovery {
> + __u8 type;
> + __le16 filter_count;
> + struct mgmt_uuid_filter filter[0];
> +} __packed;
> +
> +#define MGMT_OP_STOP_SERVICE_DISCOVERY 0x003B
> +struct mgmt_cp_stop_service_discovery {
> + __u8 type;
> +} __packed;
> +#define MGMT_STOP_SERVICE_DISCOVERY_SIZE 1
> +
> #define MGMT_EV_CMD_COMPLETE 0x0001
> struct mgmt_ev_cmd_complete {
> __le16 opcode;
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 858cf54..fab6755 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3853,6 +3853,7 @@ static void le_scan_disable_work(struct work_struct *work)
> BT_DBG("%s", hdev->name);
>
> cancel_delayed_work_sync(&hdev->le_scan_restart);
> + mgmt_clean_up_service_discovery(hdev);
>
> hci_req_init(&req, hdev);
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index f650d30..ed67060 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -93,6 +93,8 @@ static const u16 mgmt_commands[] = {
> MGMT_OP_READ_CONFIG_INFO,
> MGMT_OP_SET_EXTERNAL_CONFIG,
> MGMT_OP_SET_PUBLIC_ADDRESS,
> + MGMT_OP_START_SERVICE_DISCOVERY,
> + MGMT_OP_STOP_SERVICE_DISCOVERY,
> };
>
> static const u16 mgmt_events[] = {
> @@ -1258,6 +1260,23 @@ static void clean_up_hci_complete(struct hci_dev *hdev, u8 status)
> }
> }
>
> +void mgmt_clean_up_service_discovery(struct hci_dev *hdev)
> +{
Can you perhaps add a comment somewhere that this function in effect
cleans up the state set up by the "Start Service Discovery" function.
It's not easy to make that association just by looking at the function
name while we have a dozen functions with the name "discovery" in it
that correspond to the other mgmt device discovery command.
> + struct uuid_filter *filter;
> + struct uuid_filter *tmp;
> +
> + if (!test_and_clear_bit(HCI_SERVICE_FILTER, &hdev->dev_flags))
> + return;
> +
> + cancel_delayed_work_sync(&hdev->le_scan_restart);
> +
> + list_for_each_entry_safe(filter, tmp, &hdev->discovery.uuid_filter,
> + list) {
> + __list_del_entry(&filter->list);
> + kfree(filter);
> + }
> +}
> +
> static bool hci_stop_discovery(struct hci_request *req)
> {
> struct hci_dev *hdev = req->hdev;
> @@ -1270,6 +1289,7 @@ static bool hci_stop_discovery(struct hci_request *req)
> hci_req_add(req, HCI_OP_INQUIRY_CANCEL, 0, NULL);
> } else {
> cancel_delayed_work(&hdev->le_scan_disable);
> + mgmt_clean_up_service_discovery(hdev);
> hci_req_add_le_scan_disable(req);
> }
>
> @@ -3742,23 +3762,75 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status)
> generic_start_discovery_complete(hdev, status, MGMT_OP_START_DISCOVERY);
> }
>
> +static void start_service_discovery_complete(struct hci_dev *hdev, u8 status)
> +{
> + generic_start_discovery_complete(hdev, status,
> + MGMT_OP_START_SERVICE_DISCOVERY);
> +}
> +
> +static int init_service_discovery(struct hci_dev *hdev,
> + struct mgmt_uuid_filter *filters,
> + u16 filter_cnt)
> +{
> + int i;
> +
> + for (i = 0; i < filter_cnt; i++) {
> + struct mgmt_uuid_filter *key = &filters[i];
> + struct uuid_filter *tmp;
> +
> + tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + memcpy(tmp->uuid, key->uuid, 16);
> + tmp->rssi = key->rssi;
> + INIT_LIST_HEAD(&tmp->list);
> +
> + list_add(&tmp->list, &hdev->discovery.uuid_filter);
> + }
> + set_bit(HCI_SERVICE_FILTER, &hdev->dev_flags);
> + return 0;
> +}
> +
> static int generic_start_discovery(struct sock *sk, struct hci_dev *hdev,
> void *data, u16 len, uint16_t opcode)
> {
> - struct mgmt_cp_start_discovery *cp = data;
> struct pending_cmd *cmd;
> struct hci_cp_le_set_scan_param param_cp;
> struct hci_cp_le_set_scan_enable enable_cp;
> struct hci_cp_inquiry inq_cp;
> struct hci_request req;
> + struct mgmt_uuid_filter *serv_filters = NULL;
> /* General inquiry access code (GIAC) */
> u8 lap[3] = { 0x33, 0x8b, 0x9e };
> u8 status, own_addr_type, type;
> int err;
> + u16 serv_filter_cnt = 0;
>
> BT_DBG("%s", hdev->name);
>
> - type = cp->type;
> + if (opcode == MGMT_OP_START_SERVICE_DISCOVERY) {
> + struct mgmt_cp_start_service_discovery *cp = data;
> + u16 expected_len, filter_count;
> +
> + type = cp->type;
> + filter_count = __le16_to_cpu(cp->filter_count);
> + expected_len = sizeof(*cp) +
> + filter_count * sizeof(struct mgmt_uuid_filter);
> +
> + if (expected_len != len) {
> + return cmd_complete(sk, hdev->id, opcode,
> + MGMT_STATUS_INVALID_PARAMS, &type,
> + sizeof(type));
> + }
> +
> + serv_filters = cp->filter;
> + serv_filter_cnt = filter_count;
> + } else {
> + struct mgmt_cp_start_discovery *cp = data;
> +
> + type = cp->type;
> + }
>
> hci_dev_lock(hdev);
>
> @@ -3897,7 +3969,17 @@ static int generic_start_discovery(struct sock *sk, struct hci_dev *hdev,
> goto failed;
> }
>
> - err = hci_req_run(&req, start_discovery_complete);
> + if (serv_filters != NULL) {
> + err = init_service_discovery(hdev, serv_filters,
> + serv_filter_cnt);
> + if (err)
> + goto failed;
> + }
> +
> + if (opcode == MGMT_OP_START_SERVICE_DISCOVERY)
> + err = hci_req_run(&req, start_service_discovery_complete);
> + else
> + err = hci_req_run(&req, start_discovery_complete);
>
> if (err < 0)
> mgmt_pending_remove(cmd);
> @@ -3956,18 +4038,31 @@ static void stop_discovery_complete(struct hci_dev *hdev, u8 status)
> generic_stop_discovery_complete(hdev, status, MGMT_OP_STOP_DISCOVERY);
> }
>
> +static void stop_service_discovery_complete(struct hci_dev *hdev, u8 status)
> +{
> + generic_stop_discovery_complete(hdev, status,
> + MGMT_OP_STOP_SERVICE_DISCOVERY);
> +}
> +
> static int generic_stop_discovery(struct sock *sk, struct hci_dev *hdev,
> void *data, u16 len, uint16_t opcode)
> {
> struct pending_cmd *cmd;
> struct hci_request req;
> - struct mgmt_cp_stop_discovery *mgmt_cp = data;
> int err;
> u8 type;
>
> BT_DBG("%s", hdev->name);
>
> - type = mgmt_cp->type;
> + if (opcode == MGMT_OP_STOP_SERVICE_DISCOVERY) {
> + struct mgmt_cp_stop_service_discovery *mgmt_cp = data;
> +
> + type = mgmt_cp->type;
> + } else {
> + struct mgmt_cp_stop_discovery *mgmt_cp = data;
> +
> + type = mgmt_cp->type;
> + }
>
> hci_dev_lock(hdev);
>
> @@ -3994,7 +4089,10 @@ static int generic_stop_discovery(struct sock *sk, struct hci_dev *hdev,
>
> hci_stop_discovery(&req);
>
> - err = hci_req_run(&req, stop_discovery_complete);
> + if (opcode == MGMT_OP_STOP_SERVICE_DISCOVERY)
> + err = hci_req_run(&req, stop_service_discovery_complete);
> + else
> + err = hci_req_run(&req, stop_discovery_complete);
>
> if (!err) {
> hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
> @@ -5707,6 +5805,20 @@ unlock:
> return err;
> }
>
> +static int start_service_discovery(struct sock *sk, struct hci_dev *hdev,
> + void *data, u16 len)
> +{
> + return generic_start_discovery(sk, hdev, data, len,
> + MGMT_OP_START_SERVICE_DISCOVERY);
> +}
> +
> +static int stop_service_discovery(struct sock *sk, struct hci_dev *hdev,
> + void *data, u16 len)
> +{
> + return generic_stop_discovery(sk, hdev, data, len,
> + MGMT_OP_STOP_SERVICE_DISCOVERY);
> +}
> +
> static const struct mgmt_handler {
> int (*func) (struct sock *sk, struct hci_dev *hdev, void *data,
> u16 data_len);
> @@ -5771,6 +5883,8 @@ static const struct mgmt_handler {
> { read_config_info, false, MGMT_READ_CONFIG_INFO_SIZE },
> { set_external_config, false, MGMT_SET_EXTERNAL_CONFIG_SIZE },
> { set_public_address, false, MGMT_SET_PUBLIC_ADDRESS_SIZE },
> + { start_service_discovery, true, MGMT_START_SERVICE_DISCOVERY_SIZE },
> + { stop_service_discovery, false, MGMT_STOP_SERVICE_DISCOVERY_SIZE },
> };
>
> int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
> @@ -6837,6 +6951,135 @@ void mgmt_read_local_oob_data_complete(struct hci_dev *hdev, u8 *hash192,
> mgmt_pending_remove(cmd);
> }
>
> +struct parsed_uuid {
> + struct list_head list;
> + u8 uuid[16];
> +};
> +
> +/* this is reversed hex representation of bluetooth base uuid. We need it for
> + * service uuid parsing in eir.
> + */
> +static const u8 reverse_base_uuid[] = {
> + 0xfb, 0x34, 0x9b, 0x5f, 0x80, 0x00, 0x00, 0x80,
> + 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> +};
> +
> +static int add_uuid_to_list(struct list_head *uuids, u8 *uuid)
> +{
> + struct parsed_uuid *tmp_uuid;
> +
> + tmp_uuid = kmalloc(sizeof(*tmp_uuid), GFP_KERNEL);
> + if (tmp_uuid == NULL)
> + return -ENOMEM;
> +
> + memcpy(tmp_uuid->uuid, uuid, 16);
> + INIT_LIST_HEAD(&tmp_uuid->list);
> + list_add(&tmp_uuid->list, uuids);
> + return 0;
> +}
> +
> +static void free_uuids_list(struct list_head *uuids)
> +{
> + struct parsed_uuid *uuid, *tmp;
> +
> + list_for_each_entry_safe(uuid, tmp, uuids, list) {
> + __list_del_entry(&uuid->list);
> + kfree(uuid);
> + }
> +}
> +
> +static int eir_parse(u8 *eir, u8 eir_len, struct list_head *uuids)
> +{
> + size_t offset;
> + u8 uuid[16];
> + int i, ret;
> +
> + offset = 0;
> + while (offset < eir_len) {
> + uint8_t field_len = eir[0];
> +
> + /* Check for the end of EIR */
> + if (field_len == 0)
> + break;
> +
> + if (offset + field_len > eir_len)
> + return -EINVAL;
> +
> + switch (eir[1]) {
> + case EIR_UUID16_ALL:
> + case EIR_UUID16_SOME:
> + for (i = 0; i + 3 <= field_len; i += 2) {
> + memcpy(uuid, reverse_base_uuid, 16);
> + uuid[13] = eir[i + 3];
> + uuid[12] = eir[i + 2];
> + ret = add_uuid_to_list(uuids, uuid);
> + if (ret)
> + return ret;
> + }
> + break;
> + case EIR_UUID32_ALL:
> + case EIR_UUID32_SOME:
> + for (i = 0; i + 5 <= field_len; i += 4) {
> + memcpy(uuid, reverse_base_uuid, 16);
> + uuid[15] = eir[i + 5];
> + uuid[14] = eir[i + 4];
> + uuid[13] = eir[i + 3];
> + uuid[12] = eir[i + 2];
> + ret = add_uuid_to_list(uuids, uuid);
> + if (ret)
> + return ret;
> + }
> + break;
> + case EIR_UUID128_ALL:
> + case EIR_UUID128_SOME:
> + for (i = 0; i + 17 <= field_len; i += 16) {
> + memcpy(uuid, eir + i + 2, 16);
> + ret = add_uuid_to_list(uuids, uuid);
> + if (ret)
> + return ret;
> + }
> + break;
> + }
> +
> + offset += field_len + 1;
> + eir += field_len + 1;
> + }
> + return 0;
> +}
> +
> +enum {
> + NO_MATCH,
> + SERVICE_MATCH,
> + FULL_MATCH
> +};
> +
> +static u8 find_match(struct uuid_filter *filter, u8 uuid[16], s8 rssi)
> +{
> + if (memcmp(filter->uuid, uuid, 16) != 0)
> + return NO_MATCH;
> + if (rssi >= filter->rssi)
> + return FULL_MATCH;
> +
> + return SERVICE_MATCH;
> +}
> +
> +static u8 find_matches(struct hci_dev *hdev, s8 rssi, struct list_head *uuids)
> +{
> + struct uuid_filter *filter;
> + struct parsed_uuid *uuidptr, *tmp_uuid;
> + int match_type = NO_MATCH, tmp;
> +
> + list_for_each_entry_safe(uuidptr, tmp_uuid, uuids, list) {
> + list_for_each_entry(filter, &hdev->discovery.uuid_filter,
> + list) {
> + tmp = find_match(filter, uuidptr->uuid, rssi);
> + if (tmp > match_type)
> + match_type = tmp;
> + }
> + }
> + return match_type;
> +}
> +
> void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> u8 addr_type, u8 *dev_class, s8 rssi, u32 flags,
> u8 *eir, u16 eir_len, u8 *scan_rsp, u8 scan_rsp_len)
> @@ -6844,6 +7087,9 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> char buf[512];
> struct mgmt_ev_device_found *ev = (void *) buf;
> size_t ev_size;
> + LIST_HEAD(uuids);
> + int err = 0;
> + u8 match_type;
>
> /* Don't send events for a non-kernel initiated discovery. With
> * LE one exception is if we have pend_le_reports > 0 in which
> @@ -6882,7 +7128,31 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
> ev_size = sizeof(*ev) + eir_len + scan_rsp_len;
>
> - mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
> + if (!test_bit(HCI_SERVICE_FILTER, &hdev->dev_flags)) {
> + mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
> + return;
> + }
> +
> + err = eir_parse(eir, eir_len, &uuids);
> + if (err) {
> + free_uuids_list(&uuids);
> + return;
> + }
> +
> + match_type = find_matches(hdev, rssi, &uuids);
> + free_uuids_list(&uuids);
> +
> + if (match_type == NO_MATCH)
> + return;
> +
> + if (test_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks)) {
> + queue_delayed_work(hdev->workqueue,
> + &hdev->le_scan_restart,
> + DISCOV_LE_RESTART_DELAY);
> + }
You don't need the curly braces for the above if statement.
> + if (match_type == FULL_MATCH)
> + mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev,
> + ev_size, NULL);
> }
>
> void mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
> @@ -6915,10 +7185,18 @@ void mgmt_discovering(struct hci_dev *hdev, u8 discovering)
>
> BT_DBG("%s discovering %u", hdev->name, discovering);
>
> - if (discovering)
> + if (discovering) {
> cmd = mgmt_pending_find(MGMT_OP_START_DISCOVERY, hdev);
> - else
> + if (cmd == NULL)
> + cmd = mgmt_pending_find(MGMT_OP_START_SERVICE_DISCOVERY,
> + hdev);
> +
> + } else {
> cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, hdev);
> + if (cmd == NULL)
> + cmd = mgmt_pending_find(MGMT_OP_STOP_SERVICE_DISCOVERY,
> + hdev);
> + }
>
> if (cmd != NULL) {
> u8 type = hdev->discovery.type;
> --
> 2.1.0.rc2.206.gedb03e5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks,
Arman
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v7 3/3] Bluetooth: start and stop service discovery
@ 2014-11-20 17:56 Jakub Pawlowski
2014-11-20 23:33 ` Arman Uguray
0 siblings, 1 reply; 15+ messages in thread
From: Jakub Pawlowski @ 2014-11-20 17:56 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Jakub Pawlowski
This patch introduces start service discovery and stop service
discovery methods. The reason behind that is to enable users to find
specific services in range by UUID. Whole filtering is done in
mgmt_device_found.
Signed-off-by: Jakub Pawlowski <jpawlowski@google.com>
---
include/net/bluetooth/hci.h | 1 +
include/net/bluetooth/hci_core.h | 11 ++
include/net/bluetooth/mgmt.h | 24 ++++
net/bluetooth/hci_core.c | 1 +
net/bluetooth/mgmt.c | 296 +++++++++++++++++++++++++++++++++++++--
5 files changed, 324 insertions(+), 9 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 2f0bce2..97b0893 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -204,6 +204,7 @@ enum {
HCI_BREDR_ENABLED,
HCI_LE_SCAN_INTERRUPTED,
HCI_LE_SCAN_RESTARTING,
+ HCI_SERVICE_FILTER,
};
/* A mask for the flags that are supposed to remain when a reset happens
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 36211f9..39009f2 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -68,6 +68,7 @@ struct discovery_state {
struct list_head all; /* All devices found during inquiry */
struct list_head unknown; /* Name state not known */
struct list_head resolve; /* Name needs to be resolved */
+ struct list_head uuid_filter;
__u32 timestamp;
bdaddr_t last_adv_addr;
u8 last_adv_addr_type;
@@ -146,6 +147,12 @@ struct oob_data {
u8 rand256[16];
};
+struct uuid_filter {
+ struct list_head list;
+ s8 rssi;
+ u8 uuid[16];
+};
+
#define HCI_MAX_SHORT_NAME_LENGTH 10
/* Default LE RPA expiry time, 15 minutes */
@@ -502,6 +509,7 @@ static inline void discovery_init(struct hci_dev *hdev)
INIT_LIST_HEAD(&hdev->discovery.all);
INIT_LIST_HEAD(&hdev->discovery.unknown);
INIT_LIST_HEAD(&hdev->discovery.resolve);
+ INIT_LIST_HEAD(&hdev->discovery.uuid_filter);
}
bool hci_discovery_active(struct hci_dev *hdev);
@@ -1328,6 +1336,8 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event);
#define DISCOV_INTERLEAVED_INQUIRY_LEN 0x04
#define DISCOV_BREDR_INQUIRY_LEN 0x08
+#define DISCOV_LE_RESTART_DELAY msecs_to_jiffies(300)
+
int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
int mgmt_new_settings(struct hci_dev *hdev);
void mgmt_index_added(struct hci_dev *hdev);
@@ -1394,6 +1404,7 @@ void mgmt_new_conn_param(struct hci_dev *hdev, bdaddr_t *bdaddr,
u16 max_interval, u16 latency, u16 timeout);
void mgmt_reenable_advertising(struct hci_dev *hdev);
void mgmt_smp_complete(struct hci_conn *conn, bool complete);
+void mgmt_clean_up_service_discovery(struct hci_dev *hdev);
u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
u16 to_multiplier);
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index b391fd6..202caf7 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -495,6 +495,30 @@ struct mgmt_cp_set_public_address {
} __packed;
#define MGMT_SET_PUBLIC_ADDRESS_SIZE 6
+#define MGMT_OP_START_SERVICE_DISCOVERY 0x003A
+#define MGMT_START_SERVICE_DISCOVERY_SIZE 1
+
+#define MGMT_RANGE_NONE 0x00
+#define MGMT_RANGE_RSSI 0x01
+#define MGMT_RANGE_PATHLOSS 0x02
+
+struct mgmt_uuid_filter {
+ __s8 rssi;
+ __u8 uuid[16];
+} __packed;
+
+struct mgmt_cp_start_service_discovery {
+ __u8 type;
+ __le16 filter_count;
+ struct mgmt_uuid_filter filter[0];
+} __packed;
+
+#define MGMT_OP_STOP_SERVICE_DISCOVERY 0x003B
+struct mgmt_cp_stop_service_discovery {
+ __u8 type;
+} __packed;
+#define MGMT_STOP_SERVICE_DISCOVERY_SIZE 1
+
#define MGMT_EV_CMD_COMPLETE 0x0001
struct mgmt_ev_cmd_complete {
__le16 opcode;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 858cf54..fab6755 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3853,6 +3853,7 @@ static void le_scan_disable_work(struct work_struct *work)
BT_DBG("%s", hdev->name);
cancel_delayed_work_sync(&hdev->le_scan_restart);
+ mgmt_clean_up_service_discovery(hdev);
hci_req_init(&req, hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index f650d30..ed67060 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -93,6 +93,8 @@ static const u16 mgmt_commands[] = {
MGMT_OP_READ_CONFIG_INFO,
MGMT_OP_SET_EXTERNAL_CONFIG,
MGMT_OP_SET_PUBLIC_ADDRESS,
+ MGMT_OP_START_SERVICE_DISCOVERY,
+ MGMT_OP_STOP_SERVICE_DISCOVERY,
};
static const u16 mgmt_events[] = {
@@ -1258,6 +1260,23 @@ static void clean_up_hci_complete(struct hci_dev *hdev, u8 status)
}
}
+void mgmt_clean_up_service_discovery(struct hci_dev *hdev)
+{
+ struct uuid_filter *filter;
+ struct uuid_filter *tmp;
+
+ if (!test_and_clear_bit(HCI_SERVICE_FILTER, &hdev->dev_flags))
+ return;
+
+ cancel_delayed_work_sync(&hdev->le_scan_restart);
+
+ list_for_each_entry_safe(filter, tmp, &hdev->discovery.uuid_filter,
+ list) {
+ __list_del_entry(&filter->list);
+ kfree(filter);
+ }
+}
+
static bool hci_stop_discovery(struct hci_request *req)
{
struct hci_dev *hdev = req->hdev;
@@ -1270,6 +1289,7 @@ static bool hci_stop_discovery(struct hci_request *req)
hci_req_add(req, HCI_OP_INQUIRY_CANCEL, 0, NULL);
} else {
cancel_delayed_work(&hdev->le_scan_disable);
+ mgmt_clean_up_service_discovery(hdev);
hci_req_add_le_scan_disable(req);
}
@@ -3742,23 +3762,75 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status)
generic_start_discovery_complete(hdev, status, MGMT_OP_START_DISCOVERY);
}
+static void start_service_discovery_complete(struct hci_dev *hdev, u8 status)
+{
+ generic_start_discovery_complete(hdev, status,
+ MGMT_OP_START_SERVICE_DISCOVERY);
+}
+
+static int init_service_discovery(struct hci_dev *hdev,
+ struct mgmt_uuid_filter *filters,
+ u16 filter_cnt)
+{
+ int i;
+
+ for (i = 0; i < filter_cnt; i++) {
+ struct mgmt_uuid_filter *key = &filters[i];
+ struct uuid_filter *tmp;
+
+ tmp = kmalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ memcpy(tmp->uuid, key->uuid, 16);
+ tmp->rssi = key->rssi;
+ INIT_LIST_HEAD(&tmp->list);
+
+ list_add(&tmp->list, &hdev->discovery.uuid_filter);
+ }
+ set_bit(HCI_SERVICE_FILTER, &hdev->dev_flags);
+ return 0;
+}
+
static int generic_start_discovery(struct sock *sk, struct hci_dev *hdev,
void *data, u16 len, uint16_t opcode)
{
- struct mgmt_cp_start_discovery *cp = data;
struct pending_cmd *cmd;
struct hci_cp_le_set_scan_param param_cp;
struct hci_cp_le_set_scan_enable enable_cp;
struct hci_cp_inquiry inq_cp;
struct hci_request req;
+ struct mgmt_uuid_filter *serv_filters = NULL;
/* General inquiry access code (GIAC) */
u8 lap[3] = { 0x33, 0x8b, 0x9e };
u8 status, own_addr_type, type;
int err;
+ u16 serv_filter_cnt = 0;
BT_DBG("%s", hdev->name);
- type = cp->type;
+ if (opcode == MGMT_OP_START_SERVICE_DISCOVERY) {
+ struct mgmt_cp_start_service_discovery *cp = data;
+ u16 expected_len, filter_count;
+
+ type = cp->type;
+ filter_count = __le16_to_cpu(cp->filter_count);
+ expected_len = sizeof(*cp) +
+ filter_count * sizeof(struct mgmt_uuid_filter);
+
+ if (expected_len != len) {
+ return cmd_complete(sk, hdev->id, opcode,
+ MGMT_STATUS_INVALID_PARAMS, &type,
+ sizeof(type));
+ }
+
+ serv_filters = cp->filter;
+ serv_filter_cnt = filter_count;
+ } else {
+ struct mgmt_cp_start_discovery *cp = data;
+
+ type = cp->type;
+ }
hci_dev_lock(hdev);
@@ -3897,7 +3969,17 @@ static int generic_start_discovery(struct sock *sk, struct hci_dev *hdev,
goto failed;
}
- err = hci_req_run(&req, start_discovery_complete);
+ if (serv_filters != NULL) {
+ err = init_service_discovery(hdev, serv_filters,
+ serv_filter_cnt);
+ if (err)
+ goto failed;
+ }
+
+ if (opcode == MGMT_OP_START_SERVICE_DISCOVERY)
+ err = hci_req_run(&req, start_service_discovery_complete);
+ else
+ err = hci_req_run(&req, start_discovery_complete);
if (err < 0)
mgmt_pending_remove(cmd);
@@ -3956,18 +4038,31 @@ static void stop_discovery_complete(struct hci_dev *hdev, u8 status)
generic_stop_discovery_complete(hdev, status, MGMT_OP_STOP_DISCOVERY);
}
+static void stop_service_discovery_complete(struct hci_dev *hdev, u8 status)
+{
+ generic_stop_discovery_complete(hdev, status,
+ MGMT_OP_STOP_SERVICE_DISCOVERY);
+}
+
static int generic_stop_discovery(struct sock *sk, struct hci_dev *hdev,
void *data, u16 len, uint16_t opcode)
{
struct pending_cmd *cmd;
struct hci_request req;
- struct mgmt_cp_stop_discovery *mgmt_cp = data;
int err;
u8 type;
BT_DBG("%s", hdev->name);
- type = mgmt_cp->type;
+ if (opcode == MGMT_OP_STOP_SERVICE_DISCOVERY) {
+ struct mgmt_cp_stop_service_discovery *mgmt_cp = data;
+
+ type = mgmt_cp->type;
+ } else {
+ struct mgmt_cp_stop_discovery *mgmt_cp = data;
+
+ type = mgmt_cp->type;
+ }
hci_dev_lock(hdev);
@@ -3994,7 +4089,10 @@ static int generic_stop_discovery(struct sock *sk, struct hci_dev *hdev,
hci_stop_discovery(&req);
- err = hci_req_run(&req, stop_discovery_complete);
+ if (opcode == MGMT_OP_STOP_SERVICE_DISCOVERY)
+ err = hci_req_run(&req, stop_service_discovery_complete);
+ else
+ err = hci_req_run(&req, stop_discovery_complete);
if (!err) {
hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
@@ -5707,6 +5805,20 @@ unlock:
return err;
}
+static int start_service_discovery(struct sock *sk, struct hci_dev *hdev,
+ void *data, u16 len)
+{
+ return generic_start_discovery(sk, hdev, data, len,
+ MGMT_OP_START_SERVICE_DISCOVERY);
+}
+
+static int stop_service_discovery(struct sock *sk, struct hci_dev *hdev,
+ void *data, u16 len)
+{
+ return generic_stop_discovery(sk, hdev, data, len,
+ MGMT_OP_STOP_SERVICE_DISCOVERY);
+}
+
static const struct mgmt_handler {
int (*func) (struct sock *sk, struct hci_dev *hdev, void *data,
u16 data_len);
@@ -5771,6 +5883,8 @@ static const struct mgmt_handler {
{ read_config_info, false, MGMT_READ_CONFIG_INFO_SIZE },
{ set_external_config, false, MGMT_SET_EXTERNAL_CONFIG_SIZE },
{ set_public_address, false, MGMT_SET_PUBLIC_ADDRESS_SIZE },
+ { start_service_discovery, true, MGMT_START_SERVICE_DISCOVERY_SIZE },
+ { stop_service_discovery, false, MGMT_STOP_SERVICE_DISCOVERY_SIZE },
};
int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
@@ -6837,6 +6951,135 @@ void mgmt_read_local_oob_data_complete(struct hci_dev *hdev, u8 *hash192,
mgmt_pending_remove(cmd);
}
+struct parsed_uuid {
+ struct list_head list;
+ u8 uuid[16];
+};
+
+/* this is reversed hex representation of bluetooth base uuid. We need it for
+ * service uuid parsing in eir.
+ */
+static const u8 reverse_base_uuid[] = {
+ 0xfb, 0x34, 0x9b, 0x5f, 0x80, 0x00, 0x00, 0x80,
+ 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
+};
+
+static int add_uuid_to_list(struct list_head *uuids, u8 *uuid)
+{
+ struct parsed_uuid *tmp_uuid;
+
+ tmp_uuid = kmalloc(sizeof(*tmp_uuid), GFP_KERNEL);
+ if (tmp_uuid == NULL)
+ return -ENOMEM;
+
+ memcpy(tmp_uuid->uuid, uuid, 16);
+ INIT_LIST_HEAD(&tmp_uuid->list);
+ list_add(&tmp_uuid->list, uuids);
+ return 0;
+}
+
+static void free_uuids_list(struct list_head *uuids)
+{
+ struct parsed_uuid *uuid, *tmp;
+
+ list_for_each_entry_safe(uuid, tmp, uuids, list) {
+ __list_del_entry(&uuid->list);
+ kfree(uuid);
+ }
+}
+
+static int eir_parse(u8 *eir, u8 eir_len, struct list_head *uuids)
+{
+ size_t offset;
+ u8 uuid[16];
+ int i, ret;
+
+ offset = 0;
+ while (offset < eir_len) {
+ uint8_t field_len = eir[0];
+
+ /* Check for the end of EIR */
+ if (field_len == 0)
+ break;
+
+ if (offset + field_len > eir_len)
+ return -EINVAL;
+
+ switch (eir[1]) {
+ case EIR_UUID16_ALL:
+ case EIR_UUID16_SOME:
+ for (i = 0; i + 3 <= field_len; i += 2) {
+ memcpy(uuid, reverse_base_uuid, 16);
+ uuid[13] = eir[i + 3];
+ uuid[12] = eir[i + 2];
+ ret = add_uuid_to_list(uuids, uuid);
+ if (ret)
+ return ret;
+ }
+ break;
+ case EIR_UUID32_ALL:
+ case EIR_UUID32_SOME:
+ for (i = 0; i + 5 <= field_len; i += 4) {
+ memcpy(uuid, reverse_base_uuid, 16);
+ uuid[15] = eir[i + 5];
+ uuid[14] = eir[i + 4];
+ uuid[13] = eir[i + 3];
+ uuid[12] = eir[i + 2];
+ ret = add_uuid_to_list(uuids, uuid);
+ if (ret)
+ return ret;
+ }
+ break;
+ case EIR_UUID128_ALL:
+ case EIR_UUID128_SOME:
+ for (i = 0; i + 17 <= field_len; i += 16) {
+ memcpy(uuid, eir + i + 2, 16);
+ ret = add_uuid_to_list(uuids, uuid);
+ if (ret)
+ return ret;
+ }
+ break;
+ }
+
+ offset += field_len + 1;
+ eir += field_len + 1;
+ }
+ return 0;
+}
+
+enum {
+ NO_MATCH,
+ SERVICE_MATCH,
+ FULL_MATCH
+};
+
+static u8 find_match(struct uuid_filter *filter, u8 uuid[16], s8 rssi)
+{
+ if (memcmp(filter->uuid, uuid, 16) != 0)
+ return NO_MATCH;
+ if (rssi >= filter->rssi)
+ return FULL_MATCH;
+
+ return SERVICE_MATCH;
+}
+
+static u8 find_matches(struct hci_dev *hdev, s8 rssi, struct list_head *uuids)
+{
+ struct uuid_filter *filter;
+ struct parsed_uuid *uuidptr, *tmp_uuid;
+ int match_type = NO_MATCH, tmp;
+
+ list_for_each_entry_safe(uuidptr, tmp_uuid, uuids, list) {
+ list_for_each_entry(filter, &hdev->discovery.uuid_filter,
+ list) {
+ tmp = find_match(filter, uuidptr->uuid, rssi);
+ if (tmp > match_type)
+ match_type = tmp;
+ }
+ }
+ return match_type;
+}
+
void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
u8 addr_type, u8 *dev_class, s8 rssi, u32 flags,
u8 *eir, u16 eir_len, u8 *scan_rsp, u8 scan_rsp_len)
@@ -6844,6 +7087,9 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
char buf[512];
struct mgmt_ev_device_found *ev = (void *) buf;
size_t ev_size;
+ LIST_HEAD(uuids);
+ int err = 0;
+ u8 match_type;
/* Don't send events for a non-kernel initiated discovery. With
* LE one exception is if we have pend_le_reports > 0 in which
@@ -6882,7 +7128,31 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
ev_size = sizeof(*ev) + eir_len + scan_rsp_len;
- mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
+ if (!test_bit(HCI_SERVICE_FILTER, &hdev->dev_flags)) {
+ mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev, ev_size, NULL);
+ return;
+ }
+
+ err = eir_parse(eir, eir_len, &uuids);
+ if (err) {
+ free_uuids_list(&uuids);
+ return;
+ }
+
+ match_type = find_matches(hdev, rssi, &uuids);
+ free_uuids_list(&uuids);
+
+ if (match_type == NO_MATCH)
+ return;
+
+ if (test_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks)) {
+ queue_delayed_work(hdev->workqueue,
+ &hdev->le_scan_restart,
+ DISCOV_LE_RESTART_DELAY);
+ }
+ if (match_type == FULL_MATCH)
+ mgmt_event(MGMT_EV_DEVICE_FOUND, hdev, ev,
+ ev_size, NULL);
}
void mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
@@ -6915,10 +7185,18 @@ void mgmt_discovering(struct hci_dev *hdev, u8 discovering)
BT_DBG("%s discovering %u", hdev->name, discovering);
- if (discovering)
+ if (discovering) {
cmd = mgmt_pending_find(MGMT_OP_START_DISCOVERY, hdev);
- else
+ if (cmd == NULL)
+ cmd = mgmt_pending_find(MGMT_OP_START_SERVICE_DISCOVERY,
+ hdev);
+
+ } else {
cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, hdev);
+ if (cmd == NULL)
+ cmd = mgmt_pending_find(MGMT_OP_STOP_SERVICE_DISCOVERY,
+ hdev);
+ }
if (cmd != NULL) {
u8 type = hdev->discovery.type;
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2014-11-21 17:57 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-20 17:51 [PATCH v7 1/3] Bluetooth: add hci_restart_le_scan Jakub Pawlowski
2014-11-20 17:51 ` [PATCH v7 2/3] Bluetooth: Extract generic start and stop discovery Jakub Pawlowski
2014-11-20 22:14 ` Arman Uguray
2014-11-20 23:14 ` Jakub Pawlowski
2014-11-20 23:40 ` Arman Uguray
2014-11-20 23:55 ` Jakub Pawlowski
2014-11-20 17:51 ` [PATCH v7 3/3] Bluetooth: start and stop service discovery Jakub Pawlowski
2014-11-20 21:48 ` [PATCH v7 1/3] Bluetooth: add hci_restart_le_scan Arman Uguray
2014-11-20 22:24 ` Jakub Pawlowski
2014-11-20 23:37 ` Arman Uguray
2014-11-20 23:54 ` Jakub Pawlowski
2014-11-20 23:56 ` Arman Uguray
2014-11-20 17:56 [PATCH v7 3/3] Bluetooth: start and stop service discovery Jakub Pawlowski
2014-11-20 23:33 ` Arman Uguray
2014-11-21 17:57 ` Jakub Pawlowski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.