* [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
` (2 more replies)
0 siblings, 3 replies; 5+ 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] 5+ 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-24 21:09 ` [1/2] Bluetooth: hci_sync: Refactor add " bluez.test.bot 2022-05-26 0:45 ` [PATCH 1/2] " Luiz Augusto von Dentz 2 siblings, 0 replies; 5+ 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] 5+ messages in thread
* RE: [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-24 21:09 ` bluez.test.bot 2022-05-26 0:45 ` [PATCH 1/2] " Luiz Augusto von Dentz 2 siblings, 0 replies; 5+ messages in thread From: bluez.test.bot @ 2022-05-24 21:09 UTC (permalink / raw) To: linux-bluetooth, mmandlik [-- Attachment #1: Type: text/plain, Size: 1952 bytes --] This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=644760 ---Test result--- Test Summary: CheckPatch FAIL 2.59 seconds GitLint PASS 2.26 seconds SubjectPrefix PASS 1.84 seconds BuildKernel PASS 40.42 seconds BuildKernel32 PASS 35.72 seconds Incremental Build with patchesPASS 82.96 seconds TestRunner: Setup PASS 597.82 seconds TestRunner: l2cap-tester PASS 19.89 seconds TestRunner: bnep-tester PASS 7.36 seconds TestRunner: mgmt-tester PASS 122.66 seconds TestRunner: rfcomm-tester PASS 11.60 seconds TestRunner: sco-tester PASS 11.25 seconds TestRunner: smp-tester PASS 11.21 seconds TestRunner: userchan-tester PASS 7.72 seconds Details ############################## Test: CheckPatch - FAIL - 2.59 seconds Run checkpatch.pl script with rule in .checkpatch.conf [1/2] Bluetooth: hci_sync: Refactor add Adv Monitor\CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis #383: FILE: net/bluetooth/mgmt.c:4836: + status = __add_adv_patterns_monitor(sk, hdev, m, data, len, MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI); total: 0 errors, 0 warnings, 1 checks, 533 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/12860520.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. --- Regards, Linux Bluetooth ^ permalink raw reply [flat|nested] 5+ 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-24 21:09 ` [1/2] Bluetooth: hci_sync: Refactor add " bluez.test.bot @ 2022-05-26 0:45 ` Luiz Augusto von Dentz [not found] ` <CAGPPCLAf6BvtEvXotPfr4xgj8pPnRw5eHd7dK=z0N3jr1nr+Cg@mail.gmail.com> 2 siblings, 1 reply; 5+ 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@vger.kernel.org, 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] 5+ messages in thread
[parent not found: <CAGPPCLAf6BvtEvXotPfr4xgj8pPnRw5eHd7dK=z0N3jr1nr+Cg@mail.gmail.com>]
[parent not found: <CAGPPCLDeDpxX0avO_266CHc9XfYWEaRXN=9vQ29=sgw8cMRN5w@mail.gmail.com>]
* 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; 5+ 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@vger.kernel.org, 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] 5+ messages in thread
end of thread, other threads:[~2022-06-08 3:36 UTC | newest]
Thread overview: 5+ 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-24 21:09 ` [1/2] Bluetooth: hci_sync: Refactor add " bluez.test.bot
2022-05-26 0:45 ` [PATCH 1/2] " 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