This patch introduces new lock, that need to be acquired if you want
to use HCI_OP_SCAN_ENABLE in a series of request. Next patch introduces
le restarting that would have race condition without that.
Signed-off-by: Jakub Pawlowski <[email protected]>
---
include/net/bluetooth/hci_core.h | 6 ++++++
net/bluetooth/hci_conn.c | 4 ++++
net/bluetooth/hci_core.c | 16 ++++++++++++++--
net/bluetooth/mgmt.c | 34 ++++++++++++++++++++++++++++++++--
4 files changed, 56 insertions(+), 4 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 3c78270..55d66c6 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -343,6 +343,7 @@ struct hci_dev {
unsigned long dev_flags;
struct delayed_work le_scan_disable;
+ struct mutex le_scan_enable_lock;
__s8 adv_tx_power;
__u8 adv_data[HCI_MAX_AD_LENGTH];
@@ -875,6 +876,11 @@ static inline struct hci_dev *hci_dev_hold(struct hci_dev *d)
#define hci_dev_lock(d) mutex_lock(&d->lock)
#define hci_dev_unlock(d) mutex_unlock(&d->lock)
+/* HCI operations modifying LE scan state */
+
+#define hci_le_scan_enable_lock(d) mutex_lock(&d->le_scan_enable_lock)
+#define hci_le_scan_enable_unlock(d) mutex_unlock(&d->le_scan_enable_lock)
+
#define to_hci_dev(d) container_of(d, struct hci_dev, dev)
#define to_hci_conn(c) container_of(c, struct hci_conn, dev)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index fe18825..f5a83b9 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -633,6 +633,8 @@ static void create_le_conn_complete(struct hci_dev *hdev, u8 status)
{
struct hci_conn *conn;
+ hci_le_scan_enable_unlock(hdev);
+
if (status == 0)
return;
@@ -836,8 +838,10 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
hci_req_add_le_create_conn(&req, conn);
create_conn:
+ hci_le_scan_enable_lock(hdev);
err = hci_req_run(&req, create_le_conn_complete);
if (err) {
+ hci_le_scan_enable_unlock(hdev);
hci_conn_del(conn);
return ERR_PTR(err);
}
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 96e7321..3906d8a 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3830,6 +3830,7 @@ void hci_conn_params_clear_all(struct hci_dev *hdev)
static void inquiry_complete(struct hci_dev *hdev, u8 status)
{
+ hci_le_scan_enable_unlock(hdev);
if (status) {
BT_ERR("Failed to start inquiry: status %d", status);
@@ -3848,6 +3849,8 @@ static void le_scan_disable_work_complete(struct hci_dev *hdev, u8 status)
struct hci_cp_inquiry cp;
int err;
+ hci_le_scan_enable_unlock(hdev);
+
if (status) {
BT_ERR("Failed to disable LE scanning: status %d", status);
return;
@@ -3896,9 +3899,12 @@ static void le_scan_disable_work(struct work_struct *work)
hci_req_add_le_scan_disable(&req);
+ hci_le_scan_enable_lock(hdev);
err = hci_req_run(&req, le_scan_disable_work_complete);
- if (err)
+ if (err) {
BT_ERR("Disable LE scanning request failed: err %d", err);
+ hci_le_scan_enable_unlock(hdev);
+ }
}
static void set_random_addr(struct hci_request *req, bdaddr_t *rpa)
@@ -4067,6 +4073,7 @@ struct hci_dev *hci_alloc_dev(void)
mutex_init(&hdev->lock);
mutex_init(&hdev->req_lock);
+ mutex_init(&hdev->le_scan_enable_lock);
INIT_LIST_HEAD(&hdev->mgmt_pending);
INIT_LIST_HEAD(&hdev->blacklist);
@@ -5682,6 +5689,8 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
static void update_background_scan_complete(struct hci_dev *hdev, u8 status)
{
+ hci_le_scan_enable_unlock(hdev);
+
if (status)
BT_DBG("HCI request failed to update background scanning: "
"status 0x%2.2x", status);
@@ -5764,9 +5773,12 @@ void hci_update_background_scan(struct hci_dev *hdev)
BT_DBG("%s starting background scanning", hdev->name);
}
+ hci_le_scan_enable_lock(hdev);
err = hci_req_run(&req, update_background_scan_complete);
- if (err)
+ if (err) {
BT_ERR("Failed to run HCI request: err %d", err);
+ hci_le_scan_enable_unlock(hdev);
+ }
}
static bool disconnected_whitelist_entries(struct hci_dev *hdev)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 44b20de..b17bb65 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1255,6 +1255,8 @@ static void clean_up_hci_complete(struct hci_dev *hdev, u8 status)
{
BT_DBG("%s status 0x%02x", hdev->name, status);
+ hci_le_scan_enable_unlock(hdev);
+
if (hci_conn_count(hdev) == 0) {
cancel_delayed_work(&hdev->power_off);
queue_work(hdev->req_workqueue, &hdev->power_off.work);
@@ -1355,7 +1357,11 @@ static int clean_up_hci_state(struct hci_dev *hdev)
}
}
+ hci_le_scan_enable_lock(hdev);
err = hci_req_run(&req, clean_up_hci_complete);
+ if (err)
+ hci_le_scan_enable_unlock(hdev);
+
if (!err && discov_stopped)
hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
@@ -3828,6 +3834,8 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status)
BT_DBG("status %d", status);
+ hci_le_scan_enable_unlock(hdev);
+
hci_dev_lock(hdev);
cmd = mgmt_pending_find(MGMT_OP_START_DISCOVERY, hdev);
@@ -3923,8 +3931,10 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
goto failed;
}
+ hci_le_scan_enable_lock(hdev);
err = hci_req_run(&req, start_discovery_complete);
if (err < 0) {
+ hci_le_scan_enable_unlock(hdev);
mgmt_pending_remove(cmd);
goto failed;
}
@@ -4036,8 +4046,10 @@ static int start_service_discovery(struct sock *sk, struct hci_dev *hdev,
goto failed;
}
+ hci_le_scan_enable_lock(hdev);
err = hci_req_run(&req, start_discovery_complete);
if (err < 0) {
+ hci_le_scan_enable_unlock(hdev);
mgmt_pending_remove(cmd);
goto failed;
}
@@ -4055,6 +4067,8 @@ static void stop_discovery_complete(struct hci_dev *hdev, u8 status)
BT_DBG("status %d", status);
+ hci_le_scan_enable_unlock(hdev);
+
hci_dev_lock(hdev);
cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, hdev);
@@ -4107,7 +4121,11 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
hci_stop_discovery(&req);
+ hci_le_scan_enable_lock(hdev);
err = hci_req_run(&req, stop_discovery_complete);
+ if (err)
+ hci_le_scan_enable_unlock(hdev);
+
if (!err) {
hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
goto unlock;
@@ -4421,6 +4439,13 @@ static int set_static_address(struct sock *sk, struct hci_dev *hdev,
return err;
}
+static void set_scan_params_complete(struct hci_dev *hdev, u8 status)
+{
+ BT_DBG("%s status 0x%02x", hdev->name, status);
+
+ hci_le_scan_enable_unlock(hdev);
+}
+
static int set_scan_params(struct sock *sk, struct hci_dev *hdev,
void *data, u16 len)
{
@@ -4462,6 +4487,8 @@ static int set_scan_params(struct sock *sk, struct hci_dev *hdev,
*/
if (test_bit(HCI_LE_SCAN, &hdev->dev_flags) &&
hdev->discovery.state == DISCOVERY_STOPPED) {
+ int req_err;
+
struct hci_request req;
hci_req_init(&req, hdev);
@@ -4469,7 +4496,10 @@ static int set_scan_params(struct sock *sk, struct hci_dev *hdev,
hci_req_add_le_scan_disable(&req);
hci_req_add_le_passive_scan(&req);
- hci_req_run(&req, NULL);
+ hci_le_scan_enable_lock(hdev);
+ req_err = hci_req_run(&req, set_scan_params_complete);
+ if (req_err)
+ hci_le_scan_enable_unlock(hdev);
}
hci_dev_unlock(hdev);
@@ -4533,7 +4563,7 @@ static int set_fast_connectable(struct sock *sk, struct hci_dev *hdev,
return cmd_status(sk, hdev->id, MGMT_OP_SET_FAST_CONNECTABLE,
MGMT_STATUS_NOT_POWERED);
- if (!test_bit(HCI_CONNECTABLE, &hdev->dev_flags))
+4 if (!test_bit(HCI_CONNECTABLE, &hdev->dev_flags))
return cmd_status(sk, hdev->id, MGMT_OP_SET_FAST_CONNECTABLE,
MGMT_STATUS_REJECTED);
--
2.2.0.rc0.207.ga3a616c
On Tue, Dec 16, 2014 at 12:43 PM, Johan Hedberg <[email protected]> wrote:
> Hi Jakub,
>
> On Thu, Dec 11, 2014, Jakub Pawlowski wrote:
>> This patch introduces new lock, that need to be acquired if you want
>> to use HCI_OP_SCAN_ENABLE in a series of request. Next patch introduces
>> le restarting that would have race condition without that.
>
> This is not really the way that locks are typically used and will likely
> result in bugs in the long run. Each lock should have a well defined
> piece of data that it protects and the lock/unlock pair should cover a
> clear and contiguous section of code. Neither one of these principles
> seems to fulfilled by your design.
>
> As an example, your code would be stuck with the lock if you powered off
> the adapter while holding it (since the HCI request callbacks would not
> get called). So you'll need to find a different kind of solution here.
> It seems to me like you're looking for some sort of state tracking,
> which is something locking shouldn't be mixed with.
>
> Johan.
Hi Johan,
So I previously submitted this patch with new flag,
HCI_SCAN_RESTARTING that could be set on &hdev->flags, but Marcel
pointed that it'll also have race conditions.
I was thinking about it for some time, and I came with this solution:
Right now when you run queue of requests with hci_req_run, there's
only one callback at end, that can be accessed as req.complete.
I want to add ability to have callbacks when this queue is running,
after each operation, not only at end of it. This way I would be able
to set some special flag and then unset it right when it's needed,
only for disabling and enabling scan to modify
hci_cc_le_set_scan_enable behaviour properly.
(I assume that when I have queue of hci operations and I run
hci_req_run it can't be interrupted with other queue of operations for
one controller, is that right ?)
When it comes to hanges in code:
currently in hci_req_cmd_complete (
http://git.kernel.org/cgit/linux/kernel/git/bluetooth/bluetooth.git/tree/net/bluetooth/hci_core.c#n5357
) there's this piece of code:
/* If this was the last command in a request the complete
* callback would be found in hdev->sent_cmd instead of the
* command queue (hdev->cmd_q).
*/
if (hdev->sent_cmd) {
req_complete = bt_cb(hdev->sent_cmd)->req.complete;
if (req_complete) {
/* We must set the complete callback to NULL to
* avoid calling the callback more than once if
* this function gets called again.
*/
bt_cb(hdev->sent_cmd)->req.complete = NULL;
goto call_complete;
}
}
I want to modify it so that callback can be called after each
operation, or add some new kind of operation that would only run
callback.
What do you think about this solution ? You know this code better,
maybe you know simpler way ?
Hi Jakub,
On Thu, Dec 11, 2014, Jakub Pawlowski wrote:
> This patch introduces new lock, that need to be acquired if you want
> to use HCI_OP_SCAN_ENABLE in a series of request. Next patch introduces
> le restarting that would have race condition without that.
This is not really the way that locks are typically used and will likely
result in bugs in the long run. Each lock should have a well defined
piece of data that it protects and the lock/unlock pair should cover a
clear and contiguous section of code. Neither one of these principles
seems to fulfilled by your design.
As an example, your code would be stuck with the lock if you powered off
the adapter while holding it (since the HCI request callbacks would not
get called). So you'll need to find a different kind of solution here.
It seems to me like you're looking for some sort of state tracking,
which is something locking shouldn't be mixed with.
Johan.
When using LE_SCAN_FILTER_DUP_ENABLE, some controllers would send
advertising report from each LE device only once. That means that we
don't get any updates on RSSI value, and makes Service Discovery very
slow. This patch adds restarting scan when in Service Discovery, and
device with filtered uuid is found, but it's not in RSSI range to send
event yet. This way if device moves into range, we will quickly get RSSI
update.
Signed-off-by: Jakub Pawlowski <[email protected]>
---
include/net/bluetooth/hci_core.h | 1 +
net/bluetooth/mgmt.c | 37 ++++++++++++++++++++++++++++++++++++-
2 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 1c137ab..cee7cc6 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1359,6 +1359,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event);
#define DISCOV_INTERLEAVED_TIMEOUT 5120 /* msec */
#define DISCOV_INTERLEAVED_INQUIRY_LEN 0x04
#define DISCOV_BREDR_INQUIRY_LEN 0x08
+#define DISCOV_LE_RESTART_DELAY 200 /* msec */
int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
int mgmt_new_settings(struct hci_dev *hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index b17bb65..27b1cc6 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -7013,6 +7013,16 @@ static bool eir_has_uuids(u8 *eir, u16 eir_len, u16 uuid_count, u8 (*uuids)[16])
return false;
}
+static void restart_le_scan(struct hci_dev *hdev)
+{
+ /* If controller is not scanning we are done. */
+ if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
+ return;
+
+ queue_delayed_work(hdev->workqueue, &hdev->le_scan_restart,
+ msecs_to_jiffies(DISCOV_LE_RESTART_DELAY));
+}
+
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)
@@ -7042,7 +7052,8 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
* the results are also dropped.
*/
if (hdev->discovery.rssi != HCI_RSSI_INVALID &&
- (rssi < hdev->discovery.rssi || rssi == HCI_RSSI_INVALID))
+ (rssi == HCI_RSSI_INVALID || (rssi < hdev->discovery.rssi &&
+ !test_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks)))
return;
/* Make sure that the buffer is big enough. The 5 extra bytes
@@ -7082,6 +7093,14 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
hdev->discovery.uuids);
if (!match)
return;
+
+ /* we have service match. If duplicate filtering doesn't
+ * honour RSSI hanges, restart scan to make sure we'll
+ * get RSSI updates
+ */
+ if (test_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER,
+ &hdev->quirks))
+ restart_le_scan(hdev);
}
/* Copy EIR or advertising data into event */
@@ -7110,6 +7129,14 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
hdev->discovery.uuid_count,
hdev->discovery.uuids))
return;
+
+ /* we have service match. If duplicate filtering doesn't
+ * honour RSSI hanges, restart scan to make sure we'll
+ * get RSSI updates
+ */
+ if (test_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER,
+ &hdev->quirks))
+ restart_le_scan(hdev);
}
/* Append scan response data to event */
@@ -7123,6 +7150,14 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
return;
}
+ /* Check again wether RSSI value doesn't meet Service Discovery
+ * threshold. This might evaluate to true only if
+ * HCI_QUIRK_STRICT_DUPLICATE_FILTER is set.
+ */
+ if (hdev->discovery.rssi != HCI_RSSI_INVALID &&
+ rssi < hdev->discovery.rssi)
+ return;
+
ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
ev_size = sizeof(*ev) + eir_len + scan_rsp_len;
--
2.2.0.rc0.207.ga3a616c
Currently there is no way to restart le scan, and it's needed in
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 <[email protected]>
---
include/net/bluetooth/hci_core.h | 9 ++++++++
net/bluetooth/hci_core.c | 50 ++++++++++++++++++++++++++++++++++++++++
net/bluetooth/hci_event.c | 9 +++++---
3 files changed, 65 insertions(+), 3 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 55d66c6..1c137ab 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -56,6 +56,13 @@ struct inquiry_entry {
struct inquiry_data data;
};
+/* BR/EDR and/or LE discovery state flags: the flags defined here should
+ * represent state of discovery
+ */
+enum {
+ HCI_LE_SCAN_RESTARTING,
+};
+
struct discovery_state {
int type;
enum {
@@ -79,6 +86,7 @@ struct discovery_state {
s8 rssi;
u16 uuid_count;
u8 (*uuids)[16];
+ unsigned long flags;
};
struct hci_conn_hash {
@@ -343,6 +351,7 @@ struct hci_dev {
unsigned long dev_flags;
struct delayed_work le_scan_disable;
+ struct delayed_work le_scan_restart;
struct mutex le_scan_enable_lock;
__s8 adv_tx_power;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 3906d8a..96ee0c8 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2624,6 +2624,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);
@@ -3895,6 +3896,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);
@@ -3907,6 +3910,52 @@ static void le_scan_disable_work(struct work_struct *work)
}
}
+static void le_scan_restart_work_complete(struct hci_dev *hdev, u8 status)
+{
+ BT_DBG("%s", hdev->name);
+
+ hci_le_scan_enable_unlock(hdev);
+
+ clear_bit(HCI_LE_SCAN_RESTARTING, &hdev->discovery.flags);
+
+ if (status)
+ BT_ERR("Failed to restart LE scan: 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);
+
+ /* If controller is not scanning we are done. */
+ if (!test_bit(HCI_LE_SCAN, &hdev->dev_flags))
+ return;
+
+ hci_req_init(&req, hdev);
+
+ hci_req_add_le_scan_disable(&req);
+
+ memset(&cp, 0, sizeof(cp));
+ cp.enable = LE_SCAN_ENABLE;
+ cp.filter_dup = LE_SCAN_FILTER_DUP_ENABLE;
+ hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(cp), &cp);
+
+ set_bit(HCI_LE_SCAN_RESTARTING, &hdev->discovery.flags);
+
+ hci_le_scan_enable_lock(hdev);
+ err = hci_req_run(&req, le_scan_restart_work_complete);
+
+ if (err) {
+ BT_ERR("Restart LE scan request failed: err %d", err);
+ hci_le_scan_enable_unlock(hdev);
+ }
+}
+
static void set_random_addr(struct hci_request *req, bdaddr_t *rpa)
{
struct hci_dev *hdev = req->hdev;
@@ -4097,6 +4146,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 322abbb..cefb35d 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->discovery.flags))
+ cancel_delayed_work(&hdev->le_scan_disable);
clear_bit(HCI_LE_SCAN, &hdev->dev_flags);
--
2.2.0.rc0.207.ga3a616c