* [PATCH 1/2] Bluetooth: hci_sync: Refactor add Adv Monitor
@ 2022-05-24 20:14 Manish Mandlik
2022-05-24 20:14 ` [PATCH 2/2] Bluetooth: hci_sync: Refactor remove " Manish Mandlik
2022-05-26 0:45 ` [PATCH 1/2] Bluetooth: hci_sync: Refactor add " Luiz Augusto von Dentz
0 siblings, 2 replies; 4+ messages in thread
From: Manish Mandlik @ 2022-05-24 20:14 UTC (permalink / raw)
To: marcel, luiz.dentz
Cc: chromeos-bluetooth-upstreaming, linux-bluetooth, Manish Mandlik,
Miao-chen Chou, David S. Miller, Eric Dumazet, Jakub Kicinski,
Johan Hedberg, Paolo Abeni, linux-kernel, netdev
Make use of hci_cmd_sync_queue for adding an advertisement monitor.
Signed-off-by: Manish Mandlik <mmandlik@google.com>
Reviewed-by: Miao-chen Chou <mcchou@google.com>
---
include/net/bluetooth/hci_core.h | 5 +-
net/bluetooth/hci_core.c | 47 ++++-----
net/bluetooth/mgmt.c | 121 +++++++----------------
net/bluetooth/msft.c | 161 ++++++++-----------------------
4 files changed, 98 insertions(+), 236 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 5a52a2018b56..59953a7a6328 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1410,10 +1410,8 @@ bool hci_adv_instance_is_scannable(struct hci_dev *hdev, u8 instance);
void hci_adv_monitors_clear(struct hci_dev *hdev);
void hci_free_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
-int hci_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
-bool hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
- int *err);
+int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
bool hci_remove_single_adv_monitor(struct hci_dev *hdev, u16 handle, int *err);
bool hci_remove_all_adv_monitor(struct hci_dev *hdev, int *err);
bool hci_is_adv_monitoring(struct hci_dev *hdev);
@@ -1875,7 +1873,6 @@ void mgmt_advertising_removed(struct sock *sk, struct hci_dev *hdev,
u8 instance);
void mgmt_adv_monitor_removed(struct hci_dev *hdev, u16 handle);
int mgmt_phy_configuration_changed(struct hci_dev *hdev, struct sock *skip);
-int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
bdaddr_t *bdaddr, u8 addr_type);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 5abb2ca5b129..bbbbe3203130 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1873,11 +1873,6 @@ void hci_free_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
kfree(monitor);
}
-int hci_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status)
-{
- return mgmt_add_adv_patterns_monitor_complete(hdev, status);
-}
-
int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
{
return mgmt_remove_adv_monitor_complete(hdev, status);
@@ -1885,49 +1880,49 @@ int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
/* Assigns handle to a monitor, and if offloading is supported and power is on,
* also attempts to forward the request to the controller.
- * Returns true if request is forwarded (result is pending), false otherwise.
- * This function requires the caller holds hdev->lock.
*/
-bool hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
- int *err)
+int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
{
int min, max, handle;
+ int status = 0;
- *err = 0;
+ if (!monitor)
+ return -EINVAL;
- if (!monitor) {
- *err = -EINVAL;
- return false;
- }
+ hci_dev_lock(hdev);
min = HCI_MIN_ADV_MONITOR_HANDLE;
max = HCI_MIN_ADV_MONITOR_HANDLE + HCI_MAX_ADV_MONITOR_NUM_HANDLES;
handle = idr_alloc(&hdev->adv_monitors_idr, monitor, min, max,
GFP_KERNEL);
- if (handle < 0) {
- *err = handle;
- return false;
- }
+
+ hci_dev_unlock(hdev);
+
+ if (handle < 0)
+ return handle;
monitor->handle = handle;
if (!hdev_is_powered(hdev))
- return false;
+ return status;
switch (hci_get_adv_monitor_offload_ext(hdev)) {
case HCI_ADV_MONITOR_EXT_NONE:
- hci_update_passive_scan(hdev);
- bt_dev_dbg(hdev, "%s add monitor status %d", hdev->name, *err);
+ bt_dev_dbg(hdev, "%s add monitor %d status %d", hdev->name,
+ monitor->handle, status);
/* Message was not forwarded to controller - not an error */
- return false;
+ break;
+
case HCI_ADV_MONITOR_EXT_MSFT:
- *err = msft_add_monitor_pattern(hdev, monitor);
- bt_dev_dbg(hdev, "%s add monitor msft status %d", hdev->name,
- *err);
+ hci_req_sync_lock(hdev);
+ status = msft_add_monitor_pattern(hdev, monitor);
+ hci_req_sync_unlock(hdev);
+ bt_dev_dbg(hdev, "%s add monitor %d msft status %d", hdev->name,
+ monitor->handle, status);
break;
}
- return (*err == 0);
+ return status;
}
/* Attempts to tell the controller and free the monitor. If somehow the
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 74937a834648..d04f90698a87 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4653,75 +4653,21 @@ static int read_adv_mon_features(struct sock *sk, struct hci_dev *hdev,
return err;
}
-int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status)
-{
- struct mgmt_rp_add_adv_patterns_monitor rp;
- struct mgmt_pending_cmd *cmd;
- struct adv_monitor *monitor;
- int err = 0;
-
- hci_dev_lock(hdev);
-
- cmd = pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev);
- if (!cmd) {
- cmd = pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev);
- if (!cmd)
- goto done;
- }
-
- monitor = cmd->user_data;
- rp.monitor_handle = cpu_to_le16(monitor->handle);
-
- if (!status) {
- mgmt_adv_monitor_added(cmd->sk, hdev, monitor->handle);
- hdev->adv_monitors_cnt++;
- if (monitor->state == ADV_MONITOR_STATE_NOT_REGISTERED)
- monitor->state = ADV_MONITOR_STATE_REGISTERED;
- hci_update_passive_scan(hdev);
- }
-
- err = mgmt_cmd_complete(cmd->sk, cmd->index, cmd->opcode,
- mgmt_status(status), &rp, sizeof(rp));
- mgmt_pending_remove(cmd);
-
-done:
- hci_dev_unlock(hdev);
- bt_dev_dbg(hdev, "add monitor %d complete, status %u",
- rp.monitor_handle, status);
-
- return err;
-}
-
static int __add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
- struct adv_monitor *m, u8 status,
- void *data, u16 len, u16 op)
+ struct adv_monitor *m, void *data,
+ u16 len, u16 op)
{
struct mgmt_rp_add_adv_patterns_monitor rp;
- struct mgmt_pending_cmd *cmd;
+ u8 status = MGMT_STATUS_SUCCESS;
int err;
- bool pending;
-
- hci_dev_lock(hdev);
-
- if (status)
- goto unlock;
if (pending_find(MGMT_OP_SET_LE, hdev) ||
- pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev) ||
- pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev) ||
pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev)) {
status = MGMT_STATUS_BUSY;
- goto unlock;
- }
-
- cmd = mgmt_pending_add(sk, op, hdev, data, len);
- if (!cmd) {
- status = MGMT_STATUS_NO_RESOURCES;
- goto unlock;
+ goto done;
}
- cmd->user_data = m;
- pending = hci_add_adv_monitor(hdev, m, &err);
+ err = hci_add_adv_monitor(hdev, m);
if (err) {
if (err == -ENOSPC || err == -ENOMEM)
status = MGMT_STATUS_NO_RESOURCES;
@@ -4730,30 +4676,29 @@ static int __add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
else
status = MGMT_STATUS_FAILED;
- mgmt_pending_remove(cmd);
- goto unlock;
+ goto done;
}
- if (!pending) {
- mgmt_pending_remove(cmd);
- rp.monitor_handle = cpu_to_le16(m->handle);
- mgmt_adv_monitor_added(sk, hdev, m->handle);
- m->state = ADV_MONITOR_STATE_REGISTERED;
- hdev->adv_monitors_cnt++;
+ hci_dev_lock(hdev);
- hci_dev_unlock(hdev);
- return mgmt_cmd_complete(sk, hdev->id, op, MGMT_STATUS_SUCCESS,
- &rp, sizeof(rp));
- }
+ rp.monitor_handle = cpu_to_le16(m->handle);
+ mgmt_adv_monitor_added(sk, hdev, m->handle);
+ if (m->state == ADV_MONITOR_STATE_NOT_REGISTERED)
+ m->state = ADV_MONITOR_STATE_REGISTERED;
+ hdev->adv_monitors_cnt++;
+ hci_update_passive_scan(hdev);
hci_dev_unlock(hdev);
- return 0;
+done:
+ bt_dev_dbg(hdev, "add monitor %d complete, status %u", m->handle,
+ status);
-unlock:
- hci_free_adv_monitor(hdev, m);
- hci_dev_unlock(hdev);
- return mgmt_cmd_status(sk, hdev->id, op, status);
+ if (status)
+ return mgmt_cmd_status(sk, hdev->id, op, status);
+
+ return mgmt_cmd_complete(sk, hdev->id, op, MGMT_STATUS_SUCCESS, &rp,
+ sizeof(rp));
}
static void parse_adv_monitor_rssi(struct adv_monitor *m,
@@ -4817,7 +4762,7 @@ static int add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
{
struct mgmt_cp_add_adv_patterns_monitor *cp = data;
struct adv_monitor *m = NULL;
- u8 status = MGMT_STATUS_SUCCESS;
+ int status = MGMT_STATUS_SUCCESS;
size_t expected_size = sizeof(*cp);
BT_DBG("request for %s", hdev->name);
@@ -4843,10 +4788,14 @@ static int add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
parse_adv_monitor_rssi(m, NULL);
status = parse_adv_monitor_pattern(m, cp->pattern_count, cp->patterns);
+ if (status)
+ goto done;
+
+ status = __add_adv_patterns_monitor(sk, hdev, m, data, len,
+ MGMT_OP_ADD_ADV_PATTERNS_MONITOR);
done:
- return __add_adv_patterns_monitor(sk, hdev, m, status, data, len,
- MGMT_OP_ADD_ADV_PATTERNS_MONITOR);
+ return status;
}
static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
@@ -4854,7 +4803,7 @@ static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
{
struct mgmt_cp_add_adv_patterns_monitor_rssi *cp = data;
struct adv_monitor *m = NULL;
- u8 status = MGMT_STATUS_SUCCESS;
+ int status = MGMT_STATUS_SUCCESS;
size_t expected_size = sizeof(*cp);
BT_DBG("request for %s", hdev->name);
@@ -4880,10 +4829,14 @@ static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
parse_adv_monitor_rssi(m, &cp->rssi);
status = parse_adv_monitor_pattern(m, cp->pattern_count, cp->patterns);
+ if (status)
+ goto done;
-done:
- return __add_adv_patterns_monitor(sk, hdev, m, status, data, len,
+ status = __add_adv_patterns_monitor(sk, hdev, m, data, len,
MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI);
+
+done:
+ return status;
}
int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
@@ -4933,9 +4886,7 @@ static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
hci_dev_lock(hdev);
if (pending_find(MGMT_OP_SET_LE, hdev) ||
- pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev) ||
- pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev) ||
- pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev)) {
+ pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev)) {
status = MGMT_STATUS_BUSY;
goto unlock;
}
diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
index f43994523b1f..9abea16c4305 100644
--- a/net/bluetooth/msft.c
+++ b/net/bluetooth/msft.c
@@ -106,8 +106,6 @@ struct msft_data {
__u8 filter_enabled;
};
-static int __msft_add_monitor_pattern(struct hci_dev *hdev,
- struct adv_monitor *monitor);
static int __msft_remove_monitor(struct hci_dev *hdev,
struct adv_monitor *monitor, u16 handle);
@@ -164,34 +162,6 @@ static bool read_supported_features(struct hci_dev *hdev,
return false;
}
-static void reregister_monitor(struct hci_dev *hdev, int handle)
-{
- struct adv_monitor *monitor;
- struct msft_data *msft = hdev->msft_data;
- int err;
-
- while (1) {
- monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
- if (!monitor) {
- /* All monitors have been resumed */
- msft->resuming = false;
- hci_update_passive_scan(hdev);
- return;
- }
-
- msft->pending_add_handle = (u16)handle;
- err = __msft_add_monitor_pattern(hdev, monitor);
-
- /* If success, we return and wait for monitor added callback */
- if (!err)
- return;
-
- /* Otherwise remove the monitor and keep registering */
- hci_free_adv_monitor(hdev, monitor);
- handle++;
- }
-}
-
/* is_mgmt = true matches the handle exposed to userspace via mgmt.
* is_mgmt = false matches the handle used by the msft controller.
* This function requires the caller holds hdev->lock
@@ -243,14 +213,14 @@ static int msft_monitor_device_del(struct hci_dev *hdev, __u16 mgmt_handle,
return count;
}
-static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
- u8 status, u16 opcode,
- struct sk_buff *skb)
+static int msft_le_monitor_advertisement_cb(struct hci_dev *hdev, u16 opcode,
+ struct sk_buff *skb)
{
struct msft_rp_le_monitor_advertisement *rp;
struct adv_monitor *monitor;
struct msft_monitor_advertisement_handle_data *handle_data;
struct msft_data *msft = hdev->msft_data;
+ int status = 0;
hci_dev_lock(hdev);
@@ -262,15 +232,16 @@ static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
goto unlock;
}
- if (status)
- goto unlock;
-
rp = (struct msft_rp_le_monitor_advertisement *)skb->data;
if (skb->len < sizeof(*rp)) {
status = HCI_ERROR_UNSPECIFIED;
goto unlock;
}
+ status = rp->status;
+ if (status)
+ goto unlock;
+
handle_data = kmalloc(sizeof(*handle_data), GFP_KERNEL);
if (!handle_data) {
status = HCI_ERROR_UNSPECIFIED;
@@ -290,8 +261,7 @@ static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
hci_dev_unlock(hdev);
- if (!msft->resuming)
- hci_add_adv_patterns_monitor_complete(hdev, status);
+ return status;
}
static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
@@ -463,7 +433,7 @@ static int msft_add_monitor_sync(struct hci_dev *hdev,
ptrdiff_t offset = 0;
u8 pattern_count = 0;
struct sk_buff *skb;
- u8 status;
+ struct msft_data *msft = hdev->msft_data;
if (!msft_monitor_pattern_valid(monitor))
return -EINVAL;
@@ -505,20 +475,40 @@ static int msft_add_monitor_sync(struct hci_dev *hdev,
if (IS_ERR(skb))
return PTR_ERR(skb);
- status = skb->data[0];
- skb_pull(skb, 1);
+ msft->pending_add_handle = monitor->handle;
- msft_le_monitor_advertisement_cb(hdev, status, hdev->msft_opcode, skb);
+ return msft_le_monitor_advertisement_cb(hdev, hdev->msft_opcode, skb);
+}
- return status;
+static void reregister_monitor(struct hci_dev *hdev)
+{
+ struct adv_monitor *monitor;
+ struct msft_data *msft = hdev->msft_data;
+ int handle = 0;
+
+ if (!msft)
+ return;
+
+ msft->resuming = true;
+
+ while (1) {
+ monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
+ if (!monitor)
+ break;
+
+ msft_add_monitor_sync(hdev, monitor);
+
+ handle++;
+ }
+
+ /* All monitors have been reregistered */
+ msft->resuming = false;
}
/* This function requires the caller holds hci_req_sync_lock */
int msft_resume_sync(struct hci_dev *hdev)
{
struct msft_data *msft = hdev->msft_data;
- struct adv_monitor *monitor;
- int handle = 0;
if (!msft || !msft_monitor_supported(hdev))
return 0;
@@ -533,24 +523,12 @@ int msft_resume_sync(struct hci_dev *hdev)
hci_dev_unlock(hdev);
- msft->resuming = true;
-
- while (1) {
- monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
- if (!monitor)
- break;
-
- msft_add_monitor_sync(hdev, monitor);
-
- handle++;
- }
-
- /* All monitors have been resumed */
- msft->resuming = false;
+ reregister_monitor(hdev);
return 0;
}
+/* This function requires the caller holds hci_req_sync_lock */
void msft_do_open(struct hci_dev *hdev)
{
struct msft_data *msft = hdev->msft_data;
@@ -583,7 +561,7 @@ void msft_do_open(struct hci_dev *hdev)
/* Monitors get removed on power off, so we need to explicitly
* tell the controller to re-monitor.
*/
- reregister_monitor(hdev, 0);
+ reregister_monitor(hdev);
}
}
@@ -829,66 +807,7 @@ static void msft_le_set_advertisement_filter_enable_cb(struct hci_dev *hdev,
hci_dev_unlock(hdev);
}
-/* This function requires the caller holds hdev->lock */
-static int __msft_add_monitor_pattern(struct hci_dev *hdev,
- struct adv_monitor *monitor)
-{
- struct msft_cp_le_monitor_advertisement *cp;
- struct msft_le_monitor_advertisement_pattern_data *pattern_data;
- struct msft_le_monitor_advertisement_pattern *pattern;
- struct adv_pattern *entry;
- struct hci_request req;
- struct msft_data *msft = hdev->msft_data;
- size_t total_size = sizeof(*cp) + sizeof(*pattern_data);
- ptrdiff_t offset = 0;
- u8 pattern_count = 0;
- int err = 0;
-
- if (!msft_monitor_pattern_valid(monitor))
- return -EINVAL;
-
- list_for_each_entry(entry, &monitor->patterns, list) {
- pattern_count++;
- total_size += sizeof(*pattern) + entry->length;
- }
-
- cp = kmalloc(total_size, GFP_KERNEL);
- if (!cp)
- return -ENOMEM;
-
- cp->sub_opcode = MSFT_OP_LE_MONITOR_ADVERTISEMENT;
- cp->rssi_high = monitor->rssi.high_threshold;
- cp->rssi_low = monitor->rssi.low_threshold;
- cp->rssi_low_interval = (u8)monitor->rssi.low_threshold_timeout;
- cp->rssi_sampling_period = monitor->rssi.sampling_period;
-
- cp->cond_type = MSFT_MONITOR_ADVERTISEMENT_TYPE_PATTERN;
-
- pattern_data = (void *)cp->data;
- pattern_data->count = pattern_count;
-
- list_for_each_entry(entry, &monitor->patterns, list) {
- pattern = (void *)(pattern_data->data + offset);
- /* the length also includes data_type and offset */
- pattern->length = entry->length + 2;
- pattern->data_type = entry->ad_type;
- pattern->start_byte = entry->offset;
- memcpy(pattern->pattern, entry->value, entry->length);
- offset += sizeof(*pattern) + entry->length;
- }
-
- hci_req_init(&req, hdev);
- hci_req_add(&req, hdev->msft_opcode, total_size, cp);
- err = hci_req_run_skb(&req, msft_le_monitor_advertisement_cb);
- kfree(cp);
-
- if (!err)
- msft->pending_add_handle = monitor->handle;
-
- return err;
-}
-
-/* This function requires the caller holds hdev->lock */
+/* This function requires the caller holds hci_req_sync_lock */
int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor)
{
struct msft_data *msft = hdev->msft_data;
@@ -899,7 +818,7 @@ int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor)
if (msft->resuming || msft->suspending)
return -EBUSY;
- return __msft_add_monitor_pattern(hdev, monitor);
+ return msft_add_monitor_sync(hdev, monitor);
}
/* This function requires the caller holds hdev->lock */
--
2.36.1.124.g0e6072fb45-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] Bluetooth: hci_sync: Refactor remove Adv Monitor
2022-05-24 20:14 [PATCH 1/2] Bluetooth: hci_sync: Refactor add Adv Monitor Manish Mandlik
@ 2022-05-24 20:14 ` Manish Mandlik
2022-05-26 0:45 ` [PATCH 1/2] Bluetooth: hci_sync: Refactor add " Luiz Augusto von Dentz
1 sibling, 0 replies; 4+ messages in thread
From: Manish Mandlik @ 2022-05-24 20:14 UTC (permalink / raw)
To: marcel, luiz.dentz
Cc: chromeos-bluetooth-upstreaming, linux-bluetooth, Manish Mandlik,
Miao-chen Chou, David S. Miller, Eric Dumazet, Jakub Kicinski,
Johan Hedberg, Paolo Abeni, linux-kernel, netdev
Make use of hci_cmd_sync_queue for removing an advertisement monitor.
Signed-off-by: Manish Mandlik <mmandlik@google.com>
Reviewed-by: Miao-chen Chou <mcchou@google.com>
---
include/net/bluetooth/hci_core.h | 6 +--
net/bluetooth/hci_core.c | 87 ++++++++++----------------------
net/bluetooth/mgmt.c | 67 ++++++------------------
net/bluetooth/msft.c | 87 +++++++-------------------------
net/bluetooth/msft.h | 6 +--
5 files changed, 63 insertions(+), 190 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 59953a7a6328..7a1e48d794ea 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1410,10 +1410,9 @@ bool hci_adv_instance_is_scannable(struct hci_dev *hdev, u8 instance);
void hci_adv_monitors_clear(struct hci_dev *hdev);
void hci_free_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
-int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
-bool hci_remove_single_adv_monitor(struct hci_dev *hdev, u16 handle, int *err);
-bool hci_remove_all_adv_monitor(struct hci_dev *hdev, int *err);
+int hci_remove_single_adv_monitor(struct hci_dev *hdev, u16 handle);
+int hci_remove_all_adv_monitor(struct hci_dev *hdev);
bool hci_is_adv_monitoring(struct hci_dev *hdev);
int hci_get_adv_monitor_offload_ext(struct hci_dev *hdev);
@@ -1873,7 +1872,6 @@ void mgmt_advertising_removed(struct sock *sk, struct hci_dev *hdev,
u8 instance);
void mgmt_adv_monitor_removed(struct hci_dev *hdev, u16 handle);
int mgmt_phy_configuration_changed(struct hci_dev *hdev, struct sock *skip);
-int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
bdaddr_t *bdaddr, u8 addr_type);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index bbbbe3203130..c233844a3fc4 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1873,11 +1873,6 @@ void hci_free_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
kfree(monitor);
}
-int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
-{
- return mgmt_remove_adv_monitor_complete(hdev, status);
-}
-
/* Assigns handle to a monitor, and if offloading is supported and power is on,
* also attempts to forward the request to the controller.
*/
@@ -1927,92 +1922,64 @@ int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
/* Attempts to tell the controller and free the monitor. If somehow the
* controller doesn't have a corresponding handle, remove anyway.
- * Returns true if request is forwarded (result is pending), false otherwise.
- * This function requires the caller holds hdev->lock.
*/
-static bool hci_remove_adv_monitor(struct hci_dev *hdev,
- struct adv_monitor *monitor,
- u16 handle, int *err)
+static int hci_remove_adv_monitor(struct hci_dev *hdev,
+ struct adv_monitor *monitor)
{
- *err = 0;
+ int status = 0;
switch (hci_get_adv_monitor_offload_ext(hdev)) {
case HCI_ADV_MONITOR_EXT_NONE: /* also goes here when powered off */
- goto free_monitor;
+ hci_free_adv_monitor(hdev, monitor);
+ bt_dev_dbg(hdev, "%s remove monitor %d status %d", hdev->name,
+ monitor->handle, status);
+ break;
+
case HCI_ADV_MONITOR_EXT_MSFT:
- *err = msft_remove_monitor(hdev, monitor, handle);
+ hci_req_sync_lock(hdev);
+ status = msft_remove_monitor(hdev, monitor);
+ hci_req_sync_unlock(hdev);
+ bt_dev_dbg(hdev, "%s remove monitor %d msft status %d",
+ hdev->name, monitor->handle, status);
break;
}
- /* In case no matching handle registered, just free the monitor */
- if (*err == -ENOENT)
- goto free_monitor;
-
- return (*err == 0);
-
-free_monitor:
- if (*err == -ENOENT)
+ if (status == -ENOENT)
bt_dev_warn(hdev, "Removing monitor with no matching handle %d",
monitor->handle);
- hci_free_adv_monitor(hdev, monitor);
- *err = 0;
- return false;
+ return status;
}
-/* Returns true if request is forwarded (result is pending), false otherwise.
- * This function requires the caller holds hdev->lock.
- */
-bool hci_remove_single_adv_monitor(struct hci_dev *hdev, u16 handle, int *err)
+int hci_remove_single_adv_monitor(struct hci_dev *hdev, u16 handle)
{
struct adv_monitor *monitor = idr_find(&hdev->adv_monitors_idr, handle);
- bool pending;
-
- if (!monitor) {
- *err = -EINVAL;
- return false;
- }
- pending = hci_remove_adv_monitor(hdev, monitor, handle, err);
- if (!*err && !pending)
- hci_update_passive_scan(hdev);
-
- bt_dev_dbg(hdev, "%s remove monitor handle %d, status %d, %spending",
- hdev->name, handle, *err, pending ? "" : "not ");
+ if (!monitor)
+ return -EINVAL;
- return pending;
+ return hci_remove_adv_monitor(hdev, monitor);
}
-/* Returns true if request is forwarded (result is pending), false otherwise.
- * This function requires the caller holds hdev->lock.
- */
-bool hci_remove_all_adv_monitor(struct hci_dev *hdev, int *err)
+int hci_remove_all_adv_monitor(struct hci_dev *hdev)
{
struct adv_monitor *monitor;
int idr_next_id = 0;
- bool pending = false;
- bool update = false;
-
- *err = 0;
+ int status = 0;
- while (!*err && !pending) {
+ while (1) {
monitor = idr_get_next(&hdev->adv_monitors_idr, &idr_next_id);
if (!monitor)
break;
- pending = hci_remove_adv_monitor(hdev, monitor, 0, err);
+ status = hci_remove_adv_monitor(hdev, monitor);
+ if (status)
+ return status;
- if (!*err && !pending)
- update = true;
+ idr_next_id++;
}
- if (update)
- hci_update_passive_scan(hdev);
-
- bt_dev_dbg(hdev, "%s remove all monitors status %d, %spending",
- hdev->name, *err, pending ? "" : "not ");
-
- return pending;
+ return status;
}
/* This function requires the caller holds hdev->lock */
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index d04f90698a87..12d91cd87ff0 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4839,37 +4839,6 @@ static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
return status;
}
-int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
-{
- struct mgmt_rp_remove_adv_monitor rp;
- struct mgmt_cp_remove_adv_monitor *cp;
- struct mgmt_pending_cmd *cmd;
- int err = 0;
-
- hci_dev_lock(hdev);
-
- cmd = pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev);
- if (!cmd)
- goto done;
-
- cp = cmd->param;
- rp.monitor_handle = cp->monitor_handle;
-
- if (!status)
- hci_update_passive_scan(hdev);
-
- err = mgmt_cmd_complete(cmd->sk, cmd->index, cmd->opcode,
- mgmt_status(status), &rp, sizeof(rp));
- mgmt_pending_remove(cmd);
-
-done:
- hci_dev_unlock(hdev);
- bt_dev_dbg(hdev, "remove monitor %d complete, status %u",
- rp.monitor_handle, status);
-
- return err;
-}
-
static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
void *data, u16 len)
{
@@ -4877,11 +4846,7 @@ static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
struct mgmt_rp_remove_adv_monitor rp;
struct mgmt_pending_cmd *cmd;
u16 handle = __le16_to_cpu(cp->monitor_handle);
- int err, status;
- bool pending;
-
- BT_DBG("request for %s", hdev->name);
- rp.monitor_handle = cp->monitor_handle;
+ int err, status = MGMT_STATUS_SUCCESS;
hci_dev_lock(hdev);
@@ -4897,15 +4862,19 @@ static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
goto unlock;
}
+ hci_dev_unlock(hdev);
+
if (handle)
- pending = hci_remove_single_adv_monitor(hdev, handle, &err);
+ err = hci_remove_single_adv_monitor(hdev, handle);
else
- pending = hci_remove_all_adv_monitor(hdev, &err);
+ err = hci_remove_all_adv_monitor(hdev);
- if (err) {
- mgmt_pending_remove(cmd);
+ hci_dev_lock(hdev);
+
+ mgmt_pending_remove(cmd);
- if (err == -ENOENT)
+ if (err) {
+ if (err == -ENOENT || err == -EINVAL)
status = MGMT_STATUS_INVALID_INDEX;
else
status = MGMT_STATUS_FAILED;
@@ -4913,19 +4882,13 @@ static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
goto unlock;
}
- /* monitor can be removed without forwarding request to controller */
- if (!pending) {
- mgmt_pending_remove(cmd);
- hci_dev_unlock(hdev);
-
- return mgmt_cmd_complete(sk, hdev->id,
- MGMT_OP_REMOVE_ADV_MONITOR,
- MGMT_STATUS_SUCCESS,
- &rp, sizeof(rp));
- }
+ rp.monitor_handle = cp->monitor_handle;
+ hci_update_passive_scan(hdev);
hci_dev_unlock(hdev);
- return 0;
+
+ return mgmt_cmd_complete(sk, hdev->id, MGMT_OP_REMOVE_ADV_MONITOR,
+ MGMT_STATUS_SUCCESS, &rp, sizeof(rp));
unlock:
hci_dev_unlock(hdev);
diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
index 9abea16c4305..0d3378e707db 100644
--- a/net/bluetooth/msft.c
+++ b/net/bluetooth/msft.c
@@ -106,9 +106,6 @@ struct msft_data {
__u8 filter_enabled;
};
-static int __msft_remove_monitor(struct hci_dev *hdev,
- struct adv_monitor *monitor, u16 handle);
-
bool msft_monitor_supported(struct hci_dev *hdev)
{
return !!(msft_get_features(hdev) & MSFT_FEATURE_MASK_LE_ADV_MONITOR);
@@ -264,20 +261,16 @@ static int msft_le_monitor_advertisement_cb(struct hci_dev *hdev, u16 opcode,
return status;
}
-static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
- u8 status, u16 opcode,
- struct sk_buff *skb)
+static int msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
+ u16 opcode,
+ struct sk_buff *skb)
{
struct msft_cp_le_cancel_monitor_advertisement *cp;
struct msft_rp_le_cancel_monitor_advertisement *rp;
struct adv_monitor *monitor;
struct msft_monitor_advertisement_handle_data *handle_data;
struct msft_data *msft = hdev->msft_data;
- int err;
- bool pending;
-
- if (status)
- goto done;
+ int status = 0;
rp = (struct msft_rp_le_cancel_monitor_advertisement *)skb->data;
if (skb->len < sizeof(*rp)) {
@@ -285,6 +278,10 @@ static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
goto done;
}
+ status = rp->status;
+ if (status)
+ goto done;
+
hci_dev_lock(hdev);
cp = hci_sent_cmd_data(hdev, hdev->msft_opcode);
@@ -312,26 +309,10 @@ static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
kfree(handle_data);
}
- /* If remove all monitors is required, we need to continue the process
- * here because the earlier it was paused when waiting for the
- * response from controller.
- */
- if (msft->pending_remove_handle == 0) {
- pending = hci_remove_all_adv_monitor(hdev, &err);
- if (pending) {
- hci_dev_unlock(hdev);
- return;
- }
-
- if (err)
- status = HCI_ERROR_UNSPECIFIED;
- }
-
hci_dev_unlock(hdev);
done:
- if (!msft->suspending)
- hci_remove_adv_monitor_complete(hdev, status);
+ return status;
}
static int msft_remove_monitor_sync(struct hci_dev *hdev,
@@ -340,13 +321,14 @@ static int msft_remove_monitor_sync(struct hci_dev *hdev,
struct msft_cp_le_cancel_monitor_advertisement cp;
struct msft_monitor_advertisement_handle_data *handle_data;
struct sk_buff *skb;
- u8 status;
handle_data = msft_find_handle_data(hdev, monitor->handle, true);
/* If no matched handle, just remove without telling controller */
- if (!handle_data)
+ if (!handle_data) {
+ hci_free_adv_monitor(hdev, monitor);
return -ENOENT;
+ }
cp.sub_opcode = MSFT_OP_LE_CANCEL_MONITOR_ADVERTISEMENT;
cp.handle = handle_data->msft_handle;
@@ -356,13 +338,8 @@ static int msft_remove_monitor_sync(struct hci_dev *hdev,
if (IS_ERR(skb))
return PTR_ERR(skb);
- status = skb->data[0];
- skb_pull(skb, 1);
-
- msft_le_cancel_monitor_advertisement_cb(hdev, status, hdev->msft_opcode,
- skb);
-
- return status;
+ return msft_le_cancel_monitor_advertisement_cb(hdev, hdev->msft_opcode,
+ skb);
}
/* This function requires the caller holds hci_req_sync_lock */
@@ -821,38 +798,8 @@ int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor)
return msft_add_monitor_sync(hdev, monitor);
}
-/* This function requires the caller holds hdev->lock */
-static int __msft_remove_monitor(struct hci_dev *hdev,
- struct adv_monitor *monitor, u16 handle)
-{
- struct msft_cp_le_cancel_monitor_advertisement cp;
- struct msft_monitor_advertisement_handle_data *handle_data;
- struct hci_request req;
- struct msft_data *msft = hdev->msft_data;
- int err = 0;
-
- handle_data = msft_find_handle_data(hdev, monitor->handle, true);
-
- /* If no matched handle, just remove without telling controller */
- if (!handle_data)
- return -ENOENT;
-
- cp.sub_opcode = MSFT_OP_LE_CANCEL_MONITOR_ADVERTISEMENT;
- cp.handle = handle_data->msft_handle;
-
- hci_req_init(&req, hdev);
- hci_req_add(&req, hdev->msft_opcode, sizeof(cp), &cp);
- err = hci_req_run_skb(&req, msft_le_cancel_monitor_advertisement_cb);
-
- if (!err)
- msft->pending_remove_handle = handle;
-
- return err;
-}
-
-/* This function requires the caller holds hdev->lock */
-int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
- u16 handle)
+/* This function requires the caller holds hci_req_sync_lock */
+int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
{
struct msft_data *msft = hdev->msft_data;
@@ -862,7 +809,7 @@ int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
if (msft->resuming || msft->suspending)
return -EBUSY;
- return __msft_remove_monitor(hdev, monitor, handle);
+ return msft_remove_monitor_sync(hdev, monitor);
}
void msft_req_add_set_filter_enable(struct hci_request *req, bool enable)
diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
index afcaf7d3b1cb..2a63205b377b 100644
--- a/net/bluetooth/msft.h
+++ b/net/bluetooth/msft.h
@@ -20,8 +20,7 @@ void msft_do_close(struct hci_dev *hdev);
void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb);
__u64 msft_get_features(struct hci_dev *hdev);
int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor);
-int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
- u16 handle);
+int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
void msft_req_add_set_filter_enable(struct hci_request *req, bool enable);
int msft_set_filter_enable(struct hci_dev *hdev, bool enable);
int msft_suspend_sync(struct hci_dev *hdev);
@@ -49,8 +48,7 @@ static inline int msft_add_monitor_pattern(struct hci_dev *hdev,
}
static inline int msft_remove_monitor(struct hci_dev *hdev,
- struct adv_monitor *monitor,
- u16 handle)
+ struct adv_monitor *monitor)
{
return -EOPNOTSUPP;
}
--
2.36.1.124.g0e6072fb45-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] Bluetooth: hci_sync: Refactor add Adv Monitor
2022-05-24 20:14 [PATCH 1/2] Bluetooth: hci_sync: Refactor add Adv Monitor Manish Mandlik
2022-05-24 20:14 ` [PATCH 2/2] Bluetooth: hci_sync: Refactor remove " Manish Mandlik
@ 2022-05-26 0:45 ` Luiz Augusto von Dentz
[not found] ` <CAGPPCLAf6BvtEvXotPfr4xgj8pPnRw5eHd7dK=z0N3jr1nr+Cg@mail.gmail.com>
1 sibling, 1 reply; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2022-05-26 0:45 UTC (permalink / raw)
To: Manish Mandlik
Cc: Marcel Holtmann, ChromeOS Bluetooth Upstreaming, linux-bluetooth,
Miao-chen Chou, David S. Miller, Eric Dumazet, Jakub Kicinski,
Johan Hedberg, Paolo Abeni, Linux Kernel Mailing List,
open list:NETWORKING [GENERAL]
Hi Manish,
On Tue, May 24, 2022 at 1:14 PM Manish Mandlik <mmandlik@google.com> wrote:
>
> Make use of hci_cmd_sync_queue for adding an advertisement monitor.
Im a little lost here, it seems you end up not really using
hci_cmd_sync_queue are you assuming these functions are already run
from a safe context?
> Signed-off-by: Manish Mandlik <mmandlik@google.com>
> Reviewed-by: Miao-chen Chou <mcchou@google.com>
> ---
>
> include/net/bluetooth/hci_core.h | 5 +-
> net/bluetooth/hci_core.c | 47 ++++-----
> net/bluetooth/mgmt.c | 121 +++++++----------------
> net/bluetooth/msft.c | 161 ++++++++-----------------------
> 4 files changed, 98 insertions(+), 236 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 5a52a2018b56..59953a7a6328 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1410,10 +1410,8 @@ bool hci_adv_instance_is_scannable(struct hci_dev *hdev, u8 instance);
>
> void hci_adv_monitors_clear(struct hci_dev *hdev);
> void hci_free_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
> -int hci_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
> int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
> -bool hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
> - int *err);
> +int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
> bool hci_remove_single_adv_monitor(struct hci_dev *hdev, u16 handle, int *err);
> bool hci_remove_all_adv_monitor(struct hci_dev *hdev, int *err);
> bool hci_is_adv_monitoring(struct hci_dev *hdev);
> @@ -1875,7 +1873,6 @@ void mgmt_advertising_removed(struct sock *sk, struct hci_dev *hdev,
> u8 instance);
> void mgmt_adv_monitor_removed(struct hci_dev *hdev, u16 handle);
> int mgmt_phy_configuration_changed(struct hci_dev *hdev, struct sock *skip);
> -int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
> int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
> void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
> bdaddr_t *bdaddr, u8 addr_type);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 5abb2ca5b129..bbbbe3203130 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1873,11 +1873,6 @@ void hci_free_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
> kfree(monitor);
> }
>
> -int hci_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status)
> -{
> - return mgmt_add_adv_patterns_monitor_complete(hdev, status);
> -}
> -
> int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
> {
> return mgmt_remove_adv_monitor_complete(hdev, status);
> @@ -1885,49 +1880,49 @@ int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
>
> /* Assigns handle to a monitor, and if offloading is supported and power is on,
> * also attempts to forward the request to the controller.
> - * Returns true if request is forwarded (result is pending), false otherwise.
> - * This function requires the caller holds hdev->lock.
> */
> -bool hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
> - int *err)
> +int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
> {
> int min, max, handle;
> + int status = 0;
>
> - *err = 0;
> + if (!monitor)
> + return -EINVAL;
>
> - if (!monitor) {
> - *err = -EINVAL;
> - return false;
> - }
> + hci_dev_lock(hdev);
>
> min = HCI_MIN_ADV_MONITOR_HANDLE;
> max = HCI_MIN_ADV_MONITOR_HANDLE + HCI_MAX_ADV_MONITOR_NUM_HANDLES;
> handle = idr_alloc(&hdev->adv_monitors_idr, monitor, min, max,
> GFP_KERNEL);
> - if (handle < 0) {
> - *err = handle;
> - return false;
> - }
> +
> + hci_dev_unlock(hdev);
> +
> + if (handle < 0)
> + return handle;
>
> monitor->handle = handle;
>
> if (!hdev_is_powered(hdev))
> - return false;
> + return status;
>
> switch (hci_get_adv_monitor_offload_ext(hdev)) {
> case HCI_ADV_MONITOR_EXT_NONE:
> - hci_update_passive_scan(hdev);
> - bt_dev_dbg(hdev, "%s add monitor status %d", hdev->name, *err);
> + bt_dev_dbg(hdev, "%s add monitor %d status %d", hdev->name,
> + monitor->handle, status);
> /* Message was not forwarded to controller - not an error */
> - return false;
> + break;
> +
> case HCI_ADV_MONITOR_EXT_MSFT:
> - *err = msft_add_monitor_pattern(hdev, monitor);
> - bt_dev_dbg(hdev, "%s add monitor msft status %d", hdev->name,
> - *err);
> + hci_req_sync_lock(hdev);
> + status = msft_add_monitor_pattern(hdev, monitor);
> + hci_req_sync_unlock(hdev);
> + bt_dev_dbg(hdev, "%s add monitor %d msft status %d", hdev->name,
> + monitor->handle, status);
> break;
> }
>
> - return (*err == 0);
> + return status;
> }
>
> /* Attempts to tell the controller and free the monitor. If somehow the
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 74937a834648..d04f90698a87 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -4653,75 +4653,21 @@ static int read_adv_mon_features(struct sock *sk, struct hci_dev *hdev,
> return err;
> }
>
> -int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status)
> -{
> - struct mgmt_rp_add_adv_patterns_monitor rp;
> - struct mgmt_pending_cmd *cmd;
> - struct adv_monitor *monitor;
> - int err = 0;
> -
> - hci_dev_lock(hdev);
> -
> - cmd = pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev);
> - if (!cmd) {
> - cmd = pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev);
> - if (!cmd)
> - goto done;
> - }
> -
> - monitor = cmd->user_data;
> - rp.monitor_handle = cpu_to_le16(monitor->handle);
> -
> - if (!status) {
> - mgmt_adv_monitor_added(cmd->sk, hdev, monitor->handle);
> - hdev->adv_monitors_cnt++;
> - if (monitor->state == ADV_MONITOR_STATE_NOT_REGISTERED)
> - monitor->state = ADV_MONITOR_STATE_REGISTERED;
> - hci_update_passive_scan(hdev);
> - }
> -
> - err = mgmt_cmd_complete(cmd->sk, cmd->index, cmd->opcode,
> - mgmt_status(status), &rp, sizeof(rp));
> - mgmt_pending_remove(cmd);
> -
> -done:
> - hci_dev_unlock(hdev);
> - bt_dev_dbg(hdev, "add monitor %d complete, status %u",
> - rp.monitor_handle, status);
> -
> - return err;
> -}
> -
> static int __add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
> - struct adv_monitor *m, u8 status,
> - void *data, u16 len, u16 op)
> + struct adv_monitor *m, void *data,
> + u16 len, u16 op)
> {
> struct mgmt_rp_add_adv_patterns_monitor rp;
> - struct mgmt_pending_cmd *cmd;
> + u8 status = MGMT_STATUS_SUCCESS;
> int err;
> - bool pending;
> -
> - hci_dev_lock(hdev);
> -
> - if (status)
> - goto unlock;
>
> if (pending_find(MGMT_OP_SET_LE, hdev) ||
> - pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev) ||
> - pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev) ||
> pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev)) {
> status = MGMT_STATUS_BUSY;
> - goto unlock;
> - }
> -
> - cmd = mgmt_pending_add(sk, op, hdev, data, len);
> - if (!cmd) {
> - status = MGMT_STATUS_NO_RESOURCES;
> - goto unlock;
> + goto done;
> }
>
> - cmd->user_data = m;
> - pending = hci_add_adv_monitor(hdev, m, &err);
> + err = hci_add_adv_monitor(hdev, m);
> if (err) {
> if (err == -ENOSPC || err == -ENOMEM)
> status = MGMT_STATUS_NO_RESOURCES;
> @@ -4730,30 +4676,29 @@ static int __add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
> else
> status = MGMT_STATUS_FAILED;
>
> - mgmt_pending_remove(cmd);
> - goto unlock;
> + goto done;
> }
>
> - if (!pending) {
> - mgmt_pending_remove(cmd);
> - rp.monitor_handle = cpu_to_le16(m->handle);
> - mgmt_adv_monitor_added(sk, hdev, m->handle);
> - m->state = ADV_MONITOR_STATE_REGISTERED;
> - hdev->adv_monitors_cnt++;
> + hci_dev_lock(hdev);
>
> - hci_dev_unlock(hdev);
> - return mgmt_cmd_complete(sk, hdev->id, op, MGMT_STATUS_SUCCESS,
> - &rp, sizeof(rp));
> - }
> + rp.monitor_handle = cpu_to_le16(m->handle);
> + mgmt_adv_monitor_added(sk, hdev, m->handle);
> + if (m->state == ADV_MONITOR_STATE_NOT_REGISTERED)
> + m->state = ADV_MONITOR_STATE_REGISTERED;
> + hdev->adv_monitors_cnt++;
> + hci_update_passive_scan(hdev);
>
> hci_dev_unlock(hdev);
>
> - return 0;
> +done:
> + bt_dev_dbg(hdev, "add monitor %d complete, status %u", m->handle,
> + status);
>
> -unlock:
> - hci_free_adv_monitor(hdev, m);
> - hci_dev_unlock(hdev);
> - return mgmt_cmd_status(sk, hdev->id, op, status);
> + if (status)
> + return mgmt_cmd_status(sk, hdev->id, op, status);
> +
> + return mgmt_cmd_complete(sk, hdev->id, op, MGMT_STATUS_SUCCESS, &rp,
> + sizeof(rp));
> }
>
> static void parse_adv_monitor_rssi(struct adv_monitor *m,
> @@ -4817,7 +4762,7 @@ static int add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
> {
> struct mgmt_cp_add_adv_patterns_monitor *cp = data;
> struct adv_monitor *m = NULL;
> - u8 status = MGMT_STATUS_SUCCESS;
> + int status = MGMT_STATUS_SUCCESS;
> size_t expected_size = sizeof(*cp);
>
> BT_DBG("request for %s", hdev->name);
> @@ -4843,10 +4788,14 @@ static int add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
>
> parse_adv_monitor_rssi(m, NULL);
> status = parse_adv_monitor_pattern(m, cp->pattern_count, cp->patterns);
> + if (status)
> + goto done;
> +
> + status = __add_adv_patterns_monitor(sk, hdev, m, data, len,
> + MGMT_OP_ADD_ADV_PATTERNS_MONITOR);
>
> done:
> - return __add_adv_patterns_monitor(sk, hdev, m, status, data, len,
> - MGMT_OP_ADD_ADV_PATTERNS_MONITOR);
> + return status;
> }
>
> static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
> @@ -4854,7 +4803,7 @@ static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
> {
> struct mgmt_cp_add_adv_patterns_monitor_rssi *cp = data;
> struct adv_monitor *m = NULL;
> - u8 status = MGMT_STATUS_SUCCESS;
> + int status = MGMT_STATUS_SUCCESS;
> size_t expected_size = sizeof(*cp);
>
> BT_DBG("request for %s", hdev->name);
> @@ -4880,10 +4829,14 @@ static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
>
> parse_adv_monitor_rssi(m, &cp->rssi);
> status = parse_adv_monitor_pattern(m, cp->pattern_count, cp->patterns);
> + if (status)
> + goto done;
>
> -done:
> - return __add_adv_patterns_monitor(sk, hdev, m, status, data, len,
> + status = __add_adv_patterns_monitor(sk, hdev, m, data, len,
> MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI);
> +
> +done:
> + return status;
> }
>
> int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
> @@ -4933,9 +4886,7 @@ static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
> hci_dev_lock(hdev);
>
> if (pending_find(MGMT_OP_SET_LE, hdev) ||
> - pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev) ||
> - pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev) ||
> - pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev)) {
> + pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev)) {
> status = MGMT_STATUS_BUSY;
> goto unlock;
> }
> diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> index f43994523b1f..9abea16c4305 100644
> --- a/net/bluetooth/msft.c
> +++ b/net/bluetooth/msft.c
> @@ -106,8 +106,6 @@ struct msft_data {
> __u8 filter_enabled;
> };
>
> -static int __msft_add_monitor_pattern(struct hci_dev *hdev,
> - struct adv_monitor *monitor);
> static int __msft_remove_monitor(struct hci_dev *hdev,
> struct adv_monitor *monitor, u16 handle);
>
> @@ -164,34 +162,6 @@ static bool read_supported_features(struct hci_dev *hdev,
> return false;
> }
>
> -static void reregister_monitor(struct hci_dev *hdev, int handle)
> -{
> - struct adv_monitor *monitor;
> - struct msft_data *msft = hdev->msft_data;
> - int err;
> -
> - while (1) {
> - monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
> - if (!monitor) {
> - /* All monitors have been resumed */
> - msft->resuming = false;
> - hci_update_passive_scan(hdev);
> - return;
> - }
> -
> - msft->pending_add_handle = (u16)handle;
> - err = __msft_add_monitor_pattern(hdev, monitor);
> -
> - /* If success, we return and wait for monitor added callback */
> - if (!err)
> - return;
> -
> - /* Otherwise remove the monitor and keep registering */
> - hci_free_adv_monitor(hdev, monitor);
> - handle++;
> - }
> -}
> -
> /* is_mgmt = true matches the handle exposed to userspace via mgmt.
> * is_mgmt = false matches the handle used by the msft controller.
> * This function requires the caller holds hdev->lock
> @@ -243,14 +213,14 @@ static int msft_monitor_device_del(struct hci_dev *hdev, __u16 mgmt_handle,
> return count;
> }
>
> -static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
> - u8 status, u16 opcode,
> - struct sk_buff *skb)
> +static int msft_le_monitor_advertisement_cb(struct hci_dev *hdev, u16 opcode,
> + struct sk_buff *skb)
> {
> struct msft_rp_le_monitor_advertisement *rp;
> struct adv_monitor *monitor;
> struct msft_monitor_advertisement_handle_data *handle_data;
> struct msft_data *msft = hdev->msft_data;
> + int status = 0;
>
> hci_dev_lock(hdev);
>
> @@ -262,15 +232,16 @@ static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
> goto unlock;
> }
>
> - if (status)
> - goto unlock;
> -
> rp = (struct msft_rp_le_monitor_advertisement *)skb->data;
> if (skb->len < sizeof(*rp)) {
> status = HCI_ERROR_UNSPECIFIED;
> goto unlock;
> }
>
> + status = rp->status;
> + if (status)
> + goto unlock;
> +
> handle_data = kmalloc(sizeof(*handle_data), GFP_KERNEL);
> if (!handle_data) {
> status = HCI_ERROR_UNSPECIFIED;
> @@ -290,8 +261,7 @@ static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
>
> hci_dev_unlock(hdev);
>
> - if (!msft->resuming)
> - hci_add_adv_patterns_monitor_complete(hdev, status);
> + return status;
> }
>
> static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
> @@ -463,7 +433,7 @@ static int msft_add_monitor_sync(struct hci_dev *hdev,
> ptrdiff_t offset = 0;
> u8 pattern_count = 0;
> struct sk_buff *skb;
> - u8 status;
> + struct msft_data *msft = hdev->msft_data;
>
> if (!msft_monitor_pattern_valid(monitor))
> return -EINVAL;
> @@ -505,20 +475,40 @@ static int msft_add_monitor_sync(struct hci_dev *hdev,
> if (IS_ERR(skb))
> return PTR_ERR(skb);
>
> - status = skb->data[0];
> - skb_pull(skb, 1);
> + msft->pending_add_handle = monitor->handle;
>
> - msft_le_monitor_advertisement_cb(hdev, status, hdev->msft_opcode, skb);
> + return msft_le_monitor_advertisement_cb(hdev, hdev->msft_opcode, skb);
> +}
>
> - return status;
> +static void reregister_monitor(struct hci_dev *hdev)
> +{
> + struct adv_monitor *monitor;
> + struct msft_data *msft = hdev->msft_data;
> + int handle = 0;
> +
> + if (!msft)
> + return;
> +
> + msft->resuming = true;
> +
> + while (1) {
> + monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
> + if (!monitor)
> + break;
> +
> + msft_add_monitor_sync(hdev, monitor);
> +
> + handle++;
> + }
> +
> + /* All monitors have been reregistered */
> + msft->resuming = false;
> }
>
> /* This function requires the caller holds hci_req_sync_lock */
> int msft_resume_sync(struct hci_dev *hdev)
> {
> struct msft_data *msft = hdev->msft_data;
> - struct adv_monitor *monitor;
> - int handle = 0;
>
> if (!msft || !msft_monitor_supported(hdev))
> return 0;
> @@ -533,24 +523,12 @@ int msft_resume_sync(struct hci_dev *hdev)
>
> hci_dev_unlock(hdev);
>
> - msft->resuming = true;
> -
> - while (1) {
> - monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
> - if (!monitor)
> - break;
> -
> - msft_add_monitor_sync(hdev, monitor);
> -
> - handle++;
> - }
> -
> - /* All monitors have been resumed */
> - msft->resuming = false;
> + reregister_monitor(hdev);
>
> return 0;
> }
>
> +/* This function requires the caller holds hci_req_sync_lock */
> void msft_do_open(struct hci_dev *hdev)
> {
> struct msft_data *msft = hdev->msft_data;
> @@ -583,7 +561,7 @@ void msft_do_open(struct hci_dev *hdev)
> /* Monitors get removed on power off, so we need to explicitly
> * tell the controller to re-monitor.
> */
> - reregister_monitor(hdev, 0);
> + reregister_monitor(hdev);
> }
> }
>
> @@ -829,66 +807,7 @@ static void msft_le_set_advertisement_filter_enable_cb(struct hci_dev *hdev,
> hci_dev_unlock(hdev);
> }
>
> -/* This function requires the caller holds hdev->lock */
> -static int __msft_add_monitor_pattern(struct hci_dev *hdev,
> - struct adv_monitor *monitor)
> -{
> - struct msft_cp_le_monitor_advertisement *cp;
> - struct msft_le_monitor_advertisement_pattern_data *pattern_data;
> - struct msft_le_monitor_advertisement_pattern *pattern;
> - struct adv_pattern *entry;
> - struct hci_request req;
> - struct msft_data *msft = hdev->msft_data;
> - size_t total_size = sizeof(*cp) + sizeof(*pattern_data);
> - ptrdiff_t offset = 0;
> - u8 pattern_count = 0;
> - int err = 0;
> -
> - if (!msft_monitor_pattern_valid(monitor))
> - return -EINVAL;
> -
> - list_for_each_entry(entry, &monitor->patterns, list) {
> - pattern_count++;
> - total_size += sizeof(*pattern) + entry->length;
> - }
> -
> - cp = kmalloc(total_size, GFP_KERNEL);
> - if (!cp)
> - return -ENOMEM;
> -
> - cp->sub_opcode = MSFT_OP_LE_MONITOR_ADVERTISEMENT;
> - cp->rssi_high = monitor->rssi.high_threshold;
> - cp->rssi_low = monitor->rssi.low_threshold;
> - cp->rssi_low_interval = (u8)monitor->rssi.low_threshold_timeout;
> - cp->rssi_sampling_period = monitor->rssi.sampling_period;
> -
> - cp->cond_type = MSFT_MONITOR_ADVERTISEMENT_TYPE_PATTERN;
> -
> - pattern_data = (void *)cp->data;
> - pattern_data->count = pattern_count;
> -
> - list_for_each_entry(entry, &monitor->patterns, list) {
> - pattern = (void *)(pattern_data->data + offset);
> - /* the length also includes data_type and offset */
> - pattern->length = entry->length + 2;
> - pattern->data_type = entry->ad_type;
> - pattern->start_byte = entry->offset;
> - memcpy(pattern->pattern, entry->value, entry->length);
> - offset += sizeof(*pattern) + entry->length;
> - }
> -
> - hci_req_init(&req, hdev);
> - hci_req_add(&req, hdev->msft_opcode, total_size, cp);
> - err = hci_req_run_skb(&req, msft_le_monitor_advertisement_cb);
> - kfree(cp);
> -
> - if (!err)
> - msft->pending_add_handle = monitor->handle;
> -
> - return err;
> -}
> -
> -/* This function requires the caller holds hdev->lock */
> +/* This function requires the caller holds hci_req_sync_lock */
> int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor)
> {
> struct msft_data *msft = hdev->msft_data;
> @@ -899,7 +818,7 @@ int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor)
> if (msft->resuming || msft->suspending)
> return -EBUSY;
>
> - return __msft_add_monitor_pattern(hdev, monitor);
> + return msft_add_monitor_sync(hdev, monitor);
> }
>
> /* This function requires the caller holds hdev->lock */
> --
> 2.36.1.124.g0e6072fb45-goog
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] Bluetooth: hci_sync: Refactor add Adv Monitor
[not found] ` <CAGPPCLDeDpxX0avO_266CHc9XfYWEaRXN=9vQ29=sgw8cMRN5w@mail.gmail.com>
@ 2022-06-07 21:11 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2022-06-07 21:11 UTC (permalink / raw)
To: Manish Mandlik
Cc: Marcel Holtmann, ChromeOS Bluetooth Upstreaming, linux-bluetooth,
Miao-chen Chou, David S. Miller, Eric Dumazet, Jakub Kicinski,
Johan Hedberg, Paolo Abeni, Linux Kernel Mailing List,
open list:NETWORKING [GENERAL]
Hi Manish,
On Tue, Jun 7, 2022 at 9:57 AM Manish Mandlik <mmandlik@google.com> wrote:
>
> Hi Luiz,
>
> On Thu, May 26, 2022 at 7:01 PM Manish Mandlik <mmandlik@google.com> wrote:
>>
>> Hi Luiz,
>>
>>
>> On Wed, May 25, 2022 at 5:46 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>>
>>> Hi Manish,
>>>
>>> On Tue, May 24, 2022 at 1:14 PM Manish Mandlik <mmandlik@google.com> wrote:
>>> >
>>> > Make use of hci_cmd_sync_queue for adding an advertisement monitor.
>>>
>>> Im a little lost here, it seems you end up not really using
>>> hci_cmd_sync_queue are you assuming these functions are already run
>>> from a safe context?
>>
>> Not sure if I understand the question. But I am using msft_add_monitor_sync() to send a monitor to the controller which uses hci_cmd_sync_queue. It requires the caller to hold hci_req_sync_lock, which I have used at the appropriate places. Let me know if that looks correct.
It sounds like you are doing the hci_req_sync_lock from any thread
instead of using hci_cmd_sync_queue and then from its callback call
msft_add_monitor_sync, that way we guarantee only one task is
scheduling HCI traffic.
>>
>>>
>>> > Signed-off-by: Manish Mandlik <mmandlik@google.com>
>>> > Reviewed-by: Miao-chen Chou <mcchou@google.com>
>>> > ---
>>> >
>>> > include/net/bluetooth/hci_core.h | 5 +-
>>> > net/bluetooth/hci_core.c | 47 ++++-----
>>> > net/bluetooth/mgmt.c | 121 +++++++----------------
>>> > net/bluetooth/msft.c | 161 ++++++++-----------------------
>>> > 4 files changed, 98 insertions(+), 236 deletions(-)
>>> >
>>> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>> > index 5a52a2018b56..59953a7a6328 100644
>>> > --- a/include/net/bluetooth/hci_core.h
>>> > +++ b/include/net/bluetooth/hci_core.h
>>> > @@ -1410,10 +1410,8 @@ bool hci_adv_instance_is_scannable(struct hci_dev *hdev, u8 instance);
>>> >
>>> > void hci_adv_monitors_clear(struct hci_dev *hdev);
>>> > void hci_free_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
>>> > -int hci_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
>>> > int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
>>> > -bool hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
>>> > - int *err);
>>> > +int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor);
>>> > bool hci_remove_single_adv_monitor(struct hci_dev *hdev, u16 handle, int *err);
>>> > bool hci_remove_all_adv_monitor(struct hci_dev *hdev, int *err);
>>> > bool hci_is_adv_monitoring(struct hci_dev *hdev);
>>> > @@ -1875,7 +1873,6 @@ void mgmt_advertising_removed(struct sock *sk, struct hci_dev *hdev,
>>> > u8 instance);
>>> > void mgmt_adv_monitor_removed(struct hci_dev *hdev, u16 handle);
>>> > int mgmt_phy_configuration_changed(struct hci_dev *hdev, struct sock *skip);
>>> > -int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
>>> > int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
>>> > void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
>>> > bdaddr_t *bdaddr, u8 addr_type);
>>> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>> > index 5abb2ca5b129..bbbbe3203130 100644
>>> > --- a/net/bluetooth/hci_core.c
>>> > +++ b/net/bluetooth/hci_core.c
>>> > @@ -1873,11 +1873,6 @@ void hci_free_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
>>> > kfree(monitor);
>>> > }
>>> >
>>> > -int hci_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status)
>>> > -{
>>> > - return mgmt_add_adv_patterns_monitor_complete(hdev, status);
>>> > -}
>>> > -
>>> > int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
>>> > {
>>> > return mgmt_remove_adv_monitor_complete(hdev, status);
>>> > @@ -1885,49 +1880,49 @@ int hci_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
>>> >
>>> > /* Assigns handle to a monitor, and if offloading is supported and power is on,
>>> > * also attempts to forward the request to the controller.
>>> > - * Returns true if request is forwarded (result is pending), false otherwise.
>>> > - * This function requires the caller holds hdev->lock.
>>> > */
>>> > -bool hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
>>> > - int *err)
>>> > +int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor)
>>> > {
>>> > int min, max, handle;
>>> > + int status = 0;
>>> >
>>> > - *err = 0;
>>> > + if (!monitor)
>>> > + return -EINVAL;
>>> >
>>> > - if (!monitor) {
>>> > - *err = -EINVAL;
>>> > - return false;
>>> > - }
>>> > + hci_dev_lock(hdev);
>>> >
>>> > min = HCI_MIN_ADV_MONITOR_HANDLE;
>>> > max = HCI_MIN_ADV_MONITOR_HANDLE + HCI_MAX_ADV_MONITOR_NUM_HANDLES;
>>> > handle = idr_alloc(&hdev->adv_monitors_idr, monitor, min, max,
>>> > GFP_KERNEL);
>>> > - if (handle < 0) {
>>> > - *err = handle;
>>> > - return false;
>>> > - }
>>> > +
>>> > + hci_dev_unlock(hdev);
>>> > +
>>> > + if (handle < 0)
>>> > + return handle;
>>> >
>>> > monitor->handle = handle;
>>> >
>>> > if (!hdev_is_powered(hdev))
>>> > - return false;
>>> > + return status;
>>> >
>>> > switch (hci_get_adv_monitor_offload_ext(hdev)) {
>>> > case HCI_ADV_MONITOR_EXT_NONE:
>>> > - hci_update_passive_scan(hdev);
>>> > - bt_dev_dbg(hdev, "%s add monitor status %d", hdev->name, *err);
>>> > + bt_dev_dbg(hdev, "%s add monitor %d status %d", hdev->name,
>>> > + monitor->handle, status);
>>> > /* Message was not forwarded to controller - not an error */
>>> > - return false;
>>> > + break;
>>> > +
>>> > case HCI_ADV_MONITOR_EXT_MSFT:
>>> > - *err = msft_add_monitor_pattern(hdev, monitor);
>>> > - bt_dev_dbg(hdev, "%s add monitor msft status %d", hdev->name,
>>> > - *err);
>>> > + hci_req_sync_lock(hdev);
>>> > + status = msft_add_monitor_pattern(hdev, monitor);
>>> > + hci_req_sync_unlock(hdev);
>>> > + bt_dev_dbg(hdev, "%s add monitor %d msft status %d", hdev->name,
>>> > + monitor->handle, status);
>>> > break;
>>> > }
>>> >
>>> > - return (*err == 0);
>>> > + return status;
>>> > }
>>> >
>>> > /* Attempts to tell the controller and free the monitor. If somehow the
>>> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>>> > index 74937a834648..d04f90698a87 100644
>>> > --- a/net/bluetooth/mgmt.c
>>> > +++ b/net/bluetooth/mgmt.c
>>> > @@ -4653,75 +4653,21 @@ static int read_adv_mon_features(struct sock *sk, struct hci_dev *hdev,
>>> > return err;
>>> > }
>>> >
>>> > -int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status)
>>> > -{
>>> > - struct mgmt_rp_add_adv_patterns_monitor rp;
>>> > - struct mgmt_pending_cmd *cmd;
>>> > - struct adv_monitor *monitor;
>>> > - int err = 0;
>>> > -
>>> > - hci_dev_lock(hdev);
>>> > -
>>> > - cmd = pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev);
>>> > - if (!cmd) {
>>> > - cmd = pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev);
>>> > - if (!cmd)
>>> > - goto done;
>>> > - }
>>> > -
>>> > - monitor = cmd->user_data;
>>> > - rp.monitor_handle = cpu_to_le16(monitor->handle);
>>> > -
>>> > - if (!status) {
>>> > - mgmt_adv_monitor_added(cmd->sk, hdev, monitor->handle);
>>> > - hdev->adv_monitors_cnt++;
>>> > - if (monitor->state == ADV_MONITOR_STATE_NOT_REGISTERED)
>>> > - monitor->state = ADV_MONITOR_STATE_REGISTERED;
>>> > - hci_update_passive_scan(hdev);
>>> > - }
>>> > -
>>> > - err = mgmt_cmd_complete(cmd->sk, cmd->index, cmd->opcode,
>>> > - mgmt_status(status), &rp, sizeof(rp));
>>> > - mgmt_pending_remove(cmd);
>>> > -
>>> > -done:
>>> > - hci_dev_unlock(hdev);
>>> > - bt_dev_dbg(hdev, "add monitor %d complete, status %u",
>>> > - rp.monitor_handle, status);
>>> > -
>>> > - return err;
>>> > -}
>>> > -
>>> > static int __add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
>>> > - struct adv_monitor *m, u8 status,
>>> > - void *data, u16 len, u16 op)
>>> > + struct adv_monitor *m, void *data,
>>> > + u16 len, u16 op)
>>> > {
>>> > struct mgmt_rp_add_adv_patterns_monitor rp;
>>> > - struct mgmt_pending_cmd *cmd;
>>> > + u8 status = MGMT_STATUS_SUCCESS;
>>> > int err;
>>> > - bool pending;
>>> > -
>>> > - hci_dev_lock(hdev);
>>> > -
>>> > - if (status)
>>> > - goto unlock;
>>> >
>>> > if (pending_find(MGMT_OP_SET_LE, hdev) ||
>>> > - pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev) ||
>>> > - pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev) ||
>>> > pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev)) {
>>> > status = MGMT_STATUS_BUSY;
>>> > - goto unlock;
>>> > - }
>>> > -
>>> > - cmd = mgmt_pending_add(sk, op, hdev, data, len);
>>> > - if (!cmd) {
>>> > - status = MGMT_STATUS_NO_RESOURCES;
>>> > - goto unlock;
>>> > + goto done;
>>> > }
>>> >
>>> > - cmd->user_data = m;
>>> > - pending = hci_add_adv_monitor(hdev, m, &err);
>>> > + err = hci_add_adv_monitor(hdev, m);
>>> > if (err) {
>>> > if (err == -ENOSPC || err == -ENOMEM)
>>> > status = MGMT_STATUS_NO_RESOURCES;
>>> > @@ -4730,30 +4676,29 @@ static int __add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
>>> > else
>>> > status = MGMT_STATUS_FAILED;
>>> >
>>> > - mgmt_pending_remove(cmd);
>>> > - goto unlock;
>>> > + goto done;
>>> > }
>>> >
>>> > - if (!pending) {
>>> > - mgmt_pending_remove(cmd);
>>> > - rp.monitor_handle = cpu_to_le16(m->handle);
>>> > - mgmt_adv_monitor_added(sk, hdev, m->handle);
>>> > - m->state = ADV_MONITOR_STATE_REGISTERED;
>>> > - hdev->adv_monitors_cnt++;
>>> > + hci_dev_lock(hdev);
>>> >
>>> > - hci_dev_unlock(hdev);
>>> > - return mgmt_cmd_complete(sk, hdev->id, op, MGMT_STATUS_SUCCESS,
>>> > - &rp, sizeof(rp));
>>> > - }
>>> > + rp.monitor_handle = cpu_to_le16(m->handle);
>>> > + mgmt_adv_monitor_added(sk, hdev, m->handle);
>>> > + if (m->state == ADV_MONITOR_STATE_NOT_REGISTERED)
>>> > + m->state = ADV_MONITOR_STATE_REGISTERED;
>>> > + hdev->adv_monitors_cnt++;
>>> > + hci_update_passive_scan(hdev);
>>> >
>>> > hci_dev_unlock(hdev);
>>> >
>>> > - return 0;
>>> > +done:
>>> > + bt_dev_dbg(hdev, "add monitor %d complete, status %u", m->handle,
>>> > + status);
>>> >
>>> > -unlock:
>>> > - hci_free_adv_monitor(hdev, m);
>>> > - hci_dev_unlock(hdev);
>>> > - return mgmt_cmd_status(sk, hdev->id, op, status);
>>> > + if (status)
>>> > + return mgmt_cmd_status(sk, hdev->id, op, status);
>>> > +
>>> > + return mgmt_cmd_complete(sk, hdev->id, op, MGMT_STATUS_SUCCESS, &rp,
>>> > + sizeof(rp));
>>> > }
>>> >
>>> > static void parse_adv_monitor_rssi(struct adv_monitor *m,
>>> > @@ -4817,7 +4762,7 @@ static int add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
>>> > {
>>> > struct mgmt_cp_add_adv_patterns_monitor *cp = data;
>>> > struct adv_monitor *m = NULL;
>>> > - u8 status = MGMT_STATUS_SUCCESS;
>>> > + int status = MGMT_STATUS_SUCCESS;
>>> > size_t expected_size = sizeof(*cp);
>>> >
>>> > BT_DBG("request for %s", hdev->name);
>>> > @@ -4843,10 +4788,14 @@ static int add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
>>> >
>>> > parse_adv_monitor_rssi(m, NULL);
>>> > status = parse_adv_monitor_pattern(m, cp->pattern_count, cp->patterns);
>>> > + if (status)
>>> > + goto done;
>>> > +
>>> > + status = __add_adv_patterns_monitor(sk, hdev, m, data, len,
>>> > + MGMT_OP_ADD_ADV_PATTERNS_MONITOR);
>>> >
>>> > done:
>>> > - return __add_adv_patterns_monitor(sk, hdev, m, status, data, len,
>>> > - MGMT_OP_ADD_ADV_PATTERNS_MONITOR);
>>> > + return status;
>>> > }
>>> >
>>> > static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
>>> > @@ -4854,7 +4803,7 @@ static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
>>> > {
>>> > struct mgmt_cp_add_adv_patterns_monitor_rssi *cp = data;
>>> > struct adv_monitor *m = NULL;
>>> > - u8 status = MGMT_STATUS_SUCCESS;
>>> > + int status = MGMT_STATUS_SUCCESS;
>>> > size_t expected_size = sizeof(*cp);
>>> >
>>> > BT_DBG("request for %s", hdev->name);
>>> > @@ -4880,10 +4829,14 @@ static int add_adv_patterns_monitor_rssi(struct sock *sk, struct hci_dev *hdev,
>>> >
>>> > parse_adv_monitor_rssi(m, &cp->rssi);
>>> > status = parse_adv_monitor_pattern(m, cp->pattern_count, cp->patterns);
>>> > + if (status)
>>> > + goto done;
>>> >
>>> > -done:
>>> > - return __add_adv_patterns_monitor(sk, hdev, m, status, data, len,
>>> > + status = __add_adv_patterns_monitor(sk, hdev, m, data, len,
>>> > MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI);
>>> > +
>>> > +done:
>>> > + return status;
>>> > }
>>> >
>>> > int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status)
>>> > @@ -4933,9 +4886,7 @@ static int remove_adv_monitor(struct sock *sk, struct hci_dev *hdev,
>>> > hci_dev_lock(hdev);
>>> >
>>> > if (pending_find(MGMT_OP_SET_LE, hdev) ||
>>> > - pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev) ||
>>> > - pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR, hdev) ||
>>> > - pending_find(MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI, hdev)) {
>>> > + pending_find(MGMT_OP_REMOVE_ADV_MONITOR, hdev)) {
>>> > status = MGMT_STATUS_BUSY;
>>> > goto unlock;
>>> > }
>>> > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
>>> > index f43994523b1f..9abea16c4305 100644
>>> > --- a/net/bluetooth/msft.c
>>> > +++ b/net/bluetooth/msft.c
>>> > @@ -106,8 +106,6 @@ struct msft_data {
>>> > __u8 filter_enabled;
>>> > };
>>> >
>>> > -static int __msft_add_monitor_pattern(struct hci_dev *hdev,
>>> > - struct adv_monitor *monitor);
>>> > static int __msft_remove_monitor(struct hci_dev *hdev,
>>> > struct adv_monitor *monitor, u16 handle);
>>> >
>>> > @@ -164,34 +162,6 @@ static bool read_supported_features(struct hci_dev *hdev,
>>> > return false;
>>> > }
>>> >
>>> > -static void reregister_monitor(struct hci_dev *hdev, int handle)
>>> > -{
>>> > - struct adv_monitor *monitor;
>>> > - struct msft_data *msft = hdev->msft_data;
>>> > - int err;
>>> > -
>>> > - while (1) {
>>> > - monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
>>> > - if (!monitor) {
>>> > - /* All monitors have been resumed */
>>> > - msft->resuming = false;
>>> > - hci_update_passive_scan(hdev);
>>> > - return;
>>> > - }
>>> > -
>>> > - msft->pending_add_handle = (u16)handle;
>>> > - err = __msft_add_monitor_pattern(hdev, monitor);
>>> > -
>>> > - /* If success, we return and wait for monitor added callback */
>>> > - if (!err)
>>> > - return;
>>> > -
>>> > - /* Otherwise remove the monitor and keep registering */
>>> > - hci_free_adv_monitor(hdev, monitor);
>>> > - handle++;
>>> > - }
>>> > -}
>>> > -
>>> > /* is_mgmt = true matches the handle exposed to userspace via mgmt.
>>> > * is_mgmt = false matches the handle used by the msft controller.
>>> > * This function requires the caller holds hdev->lock
>>> > @@ -243,14 +213,14 @@ static int msft_monitor_device_del(struct hci_dev *hdev, __u16 mgmt_handle,
>>> > return count;
>>> > }
>>> >
>>> > -static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
>>> > - u8 status, u16 opcode,
>>> > - struct sk_buff *skb)
>>> > +static int msft_le_monitor_advertisement_cb(struct hci_dev *hdev, u16 opcode,
>>> > + struct sk_buff *skb)
>>> > {
>>> > struct msft_rp_le_monitor_advertisement *rp;
>>> > struct adv_monitor *monitor;
>>> > struct msft_monitor_advertisement_handle_data *handle_data;
>>> > struct msft_data *msft = hdev->msft_data;
>>> > + int status = 0;
>>> >
>>> > hci_dev_lock(hdev);
>>> >
>>> > @@ -262,15 +232,16 @@ static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
>>> > goto unlock;
>>> > }
>>> >
>>> > - if (status)
>>> > - goto unlock;
>>> > -
>>> > rp = (struct msft_rp_le_monitor_advertisement *)skb->data;
>>> > if (skb->len < sizeof(*rp)) {
>>> > status = HCI_ERROR_UNSPECIFIED;
>>> > goto unlock;
>>> > }
>>> >
>>> > + status = rp->status;
>>> > + if (status)
>>> > + goto unlock;
>>> > +
>>> > handle_data = kmalloc(sizeof(*handle_data), GFP_KERNEL);
>>> > if (!handle_data) {
>>> > status = HCI_ERROR_UNSPECIFIED;
>>> > @@ -290,8 +261,7 @@ static void msft_le_monitor_advertisement_cb(struct hci_dev *hdev,
>>> >
>>> > hci_dev_unlock(hdev);
>>> >
>>> > - if (!msft->resuming)
>>> > - hci_add_adv_patterns_monitor_complete(hdev, status);
>>> > + return status;
>>> > }
>>> >
>>> > static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
>>> > @@ -463,7 +433,7 @@ static int msft_add_monitor_sync(struct hci_dev *hdev,
>>> > ptrdiff_t offset = 0;
>>> > u8 pattern_count = 0;
>>> > struct sk_buff *skb;
>>> > - u8 status;
>>> > + struct msft_data *msft = hdev->msft_data;
>>> >
>>> > if (!msft_monitor_pattern_valid(monitor))
>>> > return -EINVAL;
>>> > @@ -505,20 +475,40 @@ static int msft_add_monitor_sync(struct hci_dev *hdev,
>>> > if (IS_ERR(skb))
>>> > return PTR_ERR(skb);
>>> >
>>> > - status = skb->data[0];
>>> > - skb_pull(skb, 1);
>>> > + msft->pending_add_handle = monitor->handle;
>>> >
>>> > - msft_le_monitor_advertisement_cb(hdev, status, hdev->msft_opcode, skb);
>>> > + return msft_le_monitor_advertisement_cb(hdev, hdev->msft_opcode, skb);
>>> > +}
>>> >
>>> > - return status;
>>> > +static void reregister_monitor(struct hci_dev *hdev)
>>> > +{
>>> > + struct adv_monitor *monitor;
>>> > + struct msft_data *msft = hdev->msft_data;
>>> > + int handle = 0;
>>> > +
>>> > + if (!msft)
>>> > + return;
>>> > +
>>> > + msft->resuming = true;
>>> > +
>>> > + while (1) {
>>> > + monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
>>> > + if (!monitor)
>>> > + break;
>>> > +
>>> > + msft_add_monitor_sync(hdev, monitor);
>>> > +
>>> > + handle++;
>>> > + }
>>> > +
>>> > + /* All monitors have been reregistered */
>>> > + msft->resuming = false;
>>> > }
>>> >
>>> > /* This function requires the caller holds hci_req_sync_lock */
>>> > int msft_resume_sync(struct hci_dev *hdev)
>>> > {
>>> > struct msft_data *msft = hdev->msft_data;
>>> > - struct adv_monitor *monitor;
>>> > - int handle = 0;
>>> >
>>> > if (!msft || !msft_monitor_supported(hdev))
>>> > return 0;
>>> > @@ -533,24 +523,12 @@ int msft_resume_sync(struct hci_dev *hdev)
>>> >
>>> > hci_dev_unlock(hdev);
>>> >
>>> > - msft->resuming = true;
>>> > -
>>> > - while (1) {
>>> > - monitor = idr_get_next(&hdev->adv_monitors_idr, &handle);
>>> > - if (!monitor)
>>> > - break;
>>> > -
>>> > - msft_add_monitor_sync(hdev, monitor);
>>> > -
>>> > - handle++;
>>> > - }
>>> > -
>>> > - /* All monitors have been resumed */
>>> > - msft->resuming = false;
>>> > + reregister_monitor(hdev);
>>> >
>>> > return 0;
>>> > }
>>> >
>>> > +/* This function requires the caller holds hci_req_sync_lock */
>>> > void msft_do_open(struct hci_dev *hdev)
>>> > {
>>> > struct msft_data *msft = hdev->msft_data;
>>> > @@ -583,7 +561,7 @@ void msft_do_open(struct hci_dev *hdev)
>>> > /* Monitors get removed on power off, so we need to explicitly
>>> > * tell the controller to re-monitor.
>>> > */
>>> > - reregister_monitor(hdev, 0);
>>> > + reregister_monitor(hdev);
>>> > }
>>> > }
>>> >
>>> > @@ -829,66 +807,7 @@ static void msft_le_set_advertisement_filter_enable_cb(struct hci_dev *hdev,
>>> > hci_dev_unlock(hdev);
>>> > }
>>> >
>>> > -/* This function requires the caller holds hdev->lock */
>>> > -static int __msft_add_monitor_pattern(struct hci_dev *hdev,
>>> > - struct adv_monitor *monitor)
>>> > -{
>>> > - struct msft_cp_le_monitor_advertisement *cp;
>>> > - struct msft_le_monitor_advertisement_pattern_data *pattern_data;
>>> > - struct msft_le_monitor_advertisement_pattern *pattern;
>>> > - struct adv_pattern *entry;
>>> > - struct hci_request req;
>>> > - struct msft_data *msft = hdev->msft_data;
>>> > - size_t total_size = sizeof(*cp) + sizeof(*pattern_data);
>>> > - ptrdiff_t offset = 0;
>>> > - u8 pattern_count = 0;
>>> > - int err = 0;
>>> > -
>>> > - if (!msft_monitor_pattern_valid(monitor))
>>> > - return -EINVAL;
>>> > -
>>> > - list_for_each_entry(entry, &monitor->patterns, list) {
>>> > - pattern_count++;
>>> > - total_size += sizeof(*pattern) + entry->length;
>>> > - }
>>> > -
>>> > - cp = kmalloc(total_size, GFP_KERNEL);
>>> > - if (!cp)
>>> > - return -ENOMEM;
>>> > -
>>> > - cp->sub_opcode = MSFT_OP_LE_MONITOR_ADVERTISEMENT;
>>> > - cp->rssi_high = monitor->rssi.high_threshold;
>>> > - cp->rssi_low = monitor->rssi.low_threshold;
>>> > - cp->rssi_low_interval = (u8)monitor->rssi.low_threshold_timeout;
>>> > - cp->rssi_sampling_period = monitor->rssi.sampling_period;
>>> > -
>>> > - cp->cond_type = MSFT_MONITOR_ADVERTISEMENT_TYPE_PATTERN;
>>> > -
>>> > - pattern_data = (void *)cp->data;
>>> > - pattern_data->count = pattern_count;
>>> > -
>>> > - list_for_each_entry(entry, &monitor->patterns, list) {
>>> > - pattern = (void *)(pattern_data->data + offset);
>>> > - /* the length also includes data_type and offset */
>>> > - pattern->length = entry->length + 2;
>>> > - pattern->data_type = entry->ad_type;
>>> > - pattern->start_byte = entry->offset;
>>> > - memcpy(pattern->pattern, entry->value, entry->length);
>>> > - offset += sizeof(*pattern) + entry->length;
>>> > - }
>>> > -
>>> > - hci_req_init(&req, hdev);
>>> > - hci_req_add(&req, hdev->msft_opcode, total_size, cp);
>>> > - err = hci_req_run_skb(&req, msft_le_monitor_advertisement_cb);
>>> > - kfree(cp);
>>> > -
>>> > - if (!err)
>>> > - msft->pending_add_handle = monitor->handle;
>>> > -
>>> > - return err;
>>> > -}
>>> > -
>>> > -/* This function requires the caller holds hdev->lock */
>>> > +/* This function requires the caller holds hci_req_sync_lock */
>>> > int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor)
>>> > {
>>> > struct msft_data *msft = hdev->msft_data;
>>> > @@ -899,7 +818,7 @@ int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor)
>>> > if (msft->resuming || msft->suspending)
>>> > return -EBUSY;
>>> >
>>> > - return __msft_add_monitor_pattern(hdev, monitor);
>>> > + return msft_add_monitor_sync(hdev, monitor);
>>> > }
>>> >
>>> > /* This function requires the caller holds hdev->lock */
>>> > --
>>> > 2.36.1.124.g0e6072fb45-goog
>>> >
>>>
>>>
>>> --
>>> Luiz Augusto von Dentz
>>
>>
>> Regards,
>> Manish.
>
>
> Friendly ping to review this..
>
> Regards,
> Manish.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-06-08 3:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-24 20:14 [PATCH 1/2] Bluetooth: hci_sync: Refactor add Adv Monitor Manish Mandlik
2022-05-24 20:14 ` [PATCH 2/2] Bluetooth: hci_sync: Refactor remove " Manish Mandlik
2022-05-26 0:45 ` [PATCH 1/2] Bluetooth: hci_sync: Refactor add " Luiz Augusto von Dentz
[not found] ` <CAGPPCLAf6BvtEvXotPfr4xgj8pPnRw5eHd7dK=z0N3jr1nr+Cg@mail.gmail.com>
[not found] ` <CAGPPCLDeDpxX0avO_266CHc9XfYWEaRXN=9vQ29=sgw8cMRN5w@mail.gmail.com>
2022-06-07 21:11 ` Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).