* [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, 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, 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:37 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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.