* [PATCH v9 2/6] Bluetooth: Interleave with allowlist scan
2020-11-11 7:02 [PATCH v9 1/6] Bluetooth: Replace BT_DBG with bt_dev_dbg in HCI request Howard Chung
@ 2020-11-11 7:02 ` Howard Chung
2020-11-11 11:18 ` Marcel Holtmann
2020-11-11 7:02 ` [PATCH v9 3/6] Bluetooth: Handle system suspend resume case Howard Chung
` (4 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Howard Chung @ 2020-11-11 7:02 UTC (permalink / raw)
To: linux-bluetooth, marcel, luiz.dentz
Cc: alainm, mmandlik, mcchou, Howard Chung, David S. Miller,
Jakub Kicinski, Johan Hedberg, linux-kernel, netdev
This patch implements the interleaving between allowlist scan and
no-filter scan. It'll be used to save power when at least one monitor is
registered and at least one pending connection or one device to be
scanned for.
The durations of the allowlist scan and the no-filter scan are
controlled by MGMT command: Set Default System Configuration. The
default values are set randomly for now.
Signed-off-by: Howard Chung <howardchung@google.com>
Reviewed-by: Alain Michaud <alainm@chromium.org>
Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
---
(no changes since v8)
Changes in v8:
- Simplified logic in __hci_update_interleaved_scan
- remove hdev->name when calling bt_dev_dbg
- remove 'default' in hci_req_add_le_interleaved_scan switch block
- remove {} around :1915
include/net/bluetooth/hci_core.h | 10 +++
net/bluetooth/hci_core.c | 4 +
net/bluetooth/hci_request.c | 128 +++++++++++++++++++++++++++++--
net/bluetooth/mgmt_config.c | 10 +++
4 files changed, 145 insertions(+), 7 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 9873e1c8cd163..cfede18709d8f 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -361,6 +361,8 @@ struct hci_dev {
__u8 ssp_debug_mode;
__u8 hw_error_code;
__u32 clock;
+ __u16 advmon_allowlist_duration;
+ __u16 advmon_no_filter_duration;
__u16 devid_source;
__u16 devid_vendor;
@@ -542,6 +544,14 @@ struct hci_dev {
struct delayed_work rpa_expired;
bdaddr_t rpa;
+ enum {
+ INTERLEAVE_SCAN_NONE,
+ INTERLEAVE_SCAN_NO_FILTER,
+ INTERLEAVE_SCAN_ALLOWLIST
+ } interleave_scan_state;
+
+ struct delayed_work interleave_scan;
+
#if IS_ENABLED(CONFIG_BT_LEDS)
struct led_trigger *power_led;
#endif
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 502552d6e9aff..65b7b74baba4c 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3592,6 +3592,10 @@ struct hci_dev *hci_alloc_dev(void)
hdev->cur_adv_instance = 0x00;
hdev->adv_instance_timeout = 0;
+ /* The default values will be chosen in the future */
+ hdev->advmon_allowlist_duration = 300;
+ hdev->advmon_no_filter_duration = 500;
+
hdev->sniff_max_interval = 800;
hdev->sniff_min_interval = 80;
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 048d4db9d4ea5..2fd56ee21d31f 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -378,6 +378,53 @@ void __hci_req_write_fast_connectable(struct hci_request *req, bool enable)
hci_req_add(req, HCI_OP_WRITE_PAGE_SCAN_TYPE, 1, &type);
}
+static void start_interleave_scan(struct hci_dev *hdev)
+{
+ hdev->interleave_scan_state = INTERLEAVE_SCAN_NO_FILTER;
+ queue_delayed_work(hdev->req_workqueue,
+ &hdev->interleave_scan, 0);
+}
+
+static bool is_interleave_scanning(struct hci_dev *hdev)
+{
+ return hdev->interleave_scan_state != INTERLEAVE_SCAN_NONE;
+}
+
+static void cancel_interleave_scan(struct hci_dev *hdev)
+{
+ bt_dev_dbg(hdev, "cancelling interleave scan");
+
+ cancel_delayed_work_sync(&hdev->interleave_scan);
+
+ hdev->interleave_scan_state = INTERLEAVE_SCAN_NONE;
+}
+
+/* Return true if interleave_scan wasn't started until exiting this function,
+ * otherwise, return false
+ */
+static bool __hci_update_interleaved_scan(struct hci_dev *hdev)
+{
+ /* If there is at least one ADV monitors and one pending LE connection
+ * or one device to be scanned for, we should alternate between
+ * allowlist scan and one without any filters to save power.
+ */
+ bool should_interleaving = hci_is_adv_monitoring(hdev) &&
+ !(list_empty(&hdev->pend_le_conns) &&
+ list_empty(&hdev->pend_le_reports));
+ bool is_interleaving = is_interleave_scanning(hdev);
+
+ if (should_interleaving && !is_interleaving) {
+ start_interleave_scan(hdev);
+ bt_dev_dbg(hdev, "starting interleave scan");
+ return true;
+ }
+
+ if (!should_interleaving && is_interleaving)
+ cancel_interleave_scan(hdev);
+
+ return false;
+}
+
/* This function controls the background scanning based on hdev->pend_le_conns
* list. If there are pending LE connection we start the background scanning,
* otherwise we stop it.
@@ -450,8 +497,7 @@ static void __hci_update_background_scan(struct hci_request *req)
hci_req_add_le_scan_disable(req, false);
hci_req_add_le_passive_scan(req);
-
- BT_DBG("%s starting background scanning", hdev->name);
+ bt_dev_dbg(hdev, "starting background scanning");
}
}
@@ -848,12 +894,17 @@ static u8 update_white_list(struct hci_request *req)
return 0x00;
}
- /* Once the controller offloading of advertisement monitor is in place,
- * the if condition should include the support of MSFT extension
- * support. If suspend is ongoing, whitelist should be the default to
- * prevent waking by random advertisements.
+ /* Use the allowlist unless the following conditions are all true:
+ * - We are not currently suspending
+ * - There are 1 or more ADV monitors registered
+ * - Interleaved scanning is not currently using the allowlist
+ *
+ * Once the controller offloading of advertisement monitor is in place,
+ * the above condition should include the support of MSFT extension
+ * support.
*/
- if (!idr_is_empty(&hdev->adv_monitors_idr) && !hdev->suspended)
+ if (!idr_is_empty(&hdev->adv_monitors_idr) && !hdev->suspended &&
+ hdev->interleave_scan_state != INTERLEAVE_SCAN_ALLOWLIST)
return 0x00;
/* Select filter policy to use white list */
@@ -1006,6 +1057,10 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
&own_addr_type))
return;
+ if (__hci_update_interleaved_scan(hdev))
+ return;
+
+ bt_dev_dbg(hdev, "interleave state %d", hdev->interleave_scan_state);
/* Adding or removing entries from the white list must
* happen before enabling scanning. The controller does
* not allow white list modification while scanning.
@@ -1886,6 +1941,62 @@ static void adv_timeout_expire(struct work_struct *work)
hci_dev_unlock(hdev);
}
+static int hci_req_add_le_interleaved_scan(struct hci_request *req,
+ unsigned long opt)
+{
+ struct hci_dev *hdev = req->hdev;
+ int ret = 0;
+
+ hci_dev_lock(hdev);
+
+ if (hci_dev_test_flag(hdev, HCI_LE_SCAN))
+ hci_req_add_le_scan_disable(req, false);
+ hci_req_add_le_passive_scan(req);
+
+ switch (hdev->interleave_scan_state) {
+ case INTERLEAVE_SCAN_ALLOWLIST:
+ bt_dev_dbg(hdev, "next state: allowlist");
+ hdev->interleave_scan_state = INTERLEAVE_SCAN_NO_FILTER;
+ break;
+ case INTERLEAVE_SCAN_NO_FILTER:
+ bt_dev_dbg(hdev, "next state: no filter");
+ hdev->interleave_scan_state = INTERLEAVE_SCAN_ALLOWLIST;
+ break;
+ case INTERLEAVE_SCAN_NONE:
+ BT_ERR("unexpected error");
+ ret = -1;
+ }
+
+ hci_dev_unlock(hdev);
+
+ return ret;
+}
+
+static void interleave_scan_work(struct work_struct *work)
+{
+ struct hci_dev *hdev = container_of(work, struct hci_dev,
+ interleave_scan.work);
+ u8 status;
+ unsigned long timeout;
+
+ if (hdev->interleave_scan_state == INTERLEAVE_SCAN_ALLOWLIST) {
+ timeout = msecs_to_jiffies(hdev->advmon_allowlist_duration);
+ } else if (hdev->interleave_scan_state == INTERLEAVE_SCAN_NO_FILTER) {
+ timeout = msecs_to_jiffies(hdev->advmon_no_filter_duration);
+ } else {
+ bt_dev_err(hdev, "unexpected error");
+ return;
+ }
+
+ hci_req_sync(hdev, hci_req_add_le_interleaved_scan, 0,
+ HCI_CMD_TIMEOUT, &status);
+
+ /* Don't continue interleaving if it was canceled */
+ if (is_interleave_scanning(hdev))
+ queue_delayed_work(hdev->req_workqueue,
+ &hdev->interleave_scan, timeout);
+}
+
int hci_get_random_address(struct hci_dev *hdev, bool require_privacy,
bool use_rpa, struct adv_info *adv_instance,
u8 *own_addr_type, bdaddr_t *rand_addr)
@@ -3313,6 +3424,7 @@ void hci_request_setup(struct hci_dev *hdev)
INIT_DELAYED_WORK(&hdev->le_scan_disable, le_scan_disable_work);
INIT_DELAYED_WORK(&hdev->le_scan_restart, le_scan_restart_work);
INIT_DELAYED_WORK(&hdev->adv_instance_expire, adv_timeout_expire);
+ INIT_DELAYED_WORK(&hdev->interleave_scan, interleave_scan_work);
}
void hci_request_cancel_all(struct hci_dev *hdev)
@@ -3332,4 +3444,6 @@ void hci_request_cancel_all(struct hci_dev *hdev)
cancel_delayed_work_sync(&hdev->adv_instance_expire);
hdev->adv_instance_timeout = 0;
}
+
+ cancel_interleave_scan(hdev);
}
diff --git a/net/bluetooth/mgmt_config.c b/net/bluetooth/mgmt_config.c
index b30b571f8caf8..2d3ad288c78ac 100644
--- a/net/bluetooth/mgmt_config.c
+++ b/net/bluetooth/mgmt_config.c
@@ -67,6 +67,8 @@ int read_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
HDEV_PARAM_U16(0x001a, le_supv_timeout),
HDEV_PARAM_U16_JIFFIES_TO_MSECS(0x001b,
def_le_autoconnect_timeout),
+ HDEV_PARAM_U16(0x001d, advmon_allowlist_duration),
+ HDEV_PARAM_U16(0x001e, advmon_no_filter_duration),
};
struct mgmt_rp_read_def_system_config *rp = (void *)params;
@@ -138,6 +140,8 @@ int set_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
case 0x0019:
case 0x001a:
case 0x001b:
+ case 0x001d:
+ case 0x001e:
if (len != sizeof(u16)) {
bt_dev_warn(hdev, "invalid length %d, exp %zu for type %d",
len, sizeof(u16), type);
@@ -251,6 +255,12 @@ int set_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
hdev->def_le_autoconnect_timeout =
msecs_to_jiffies(TLV_GET_LE16(buffer));
break;
+ case 0x0001d:
+ hdev->advmon_allowlist_duration = TLV_GET_LE16(buffer);
+ break;
+ case 0x0001e:
+ hdev->advmon_no_filter_duration = TLV_GET_LE16(buffer);
+ break;
default:
bt_dev_warn(hdev, "unsupported parameter %u", type);
break;
--
2.29.2.222.g5d2a92d10f8-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v9 2/6] Bluetooth: Interleave with allowlist scan
2020-11-11 7:02 ` [PATCH v9 2/6] Bluetooth: Interleave with allowlist scan Howard Chung
@ 2020-11-11 11:18 ` Marcel Holtmann
0 siblings, 0 replies; 10+ messages in thread
From: Marcel Holtmann @ 2020-11-11 11:18 UTC (permalink / raw)
To: Howard Chung
Cc: linux-bluetooth, Luiz Augusto von Dentz, alainm, mmandlik,
mcchou, David S. Miller, Jakub Kicinski, Johan Hedberg,
linux-kernel, netdev
Hi Howard,
> This patch implements the interleaving between allowlist scan and
> no-filter scan. It'll be used to save power when at least one monitor is
> registered and at least one pending connection or one device to be
> scanned for.
>
> The durations of the allowlist scan and the no-filter scan are
> controlled by MGMT command: Set Default System Configuration. The
> default values are set randomly for now.
>
> Signed-off-by: Howard Chung <howardchung@google.com>
> Reviewed-by: Alain Michaud <alainm@chromium.org>
> Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
> ---
>
> (no changes since v8)
>
> Changes in v8:
> - Simplified logic in __hci_update_interleaved_scan
> - remove hdev->name when calling bt_dev_dbg
> - remove 'default' in hci_req_add_le_interleaved_scan switch block
> - remove {} around :1915
>
> include/net/bluetooth/hci_core.h | 10 +++
> net/bluetooth/hci_core.c | 4 +
> net/bluetooth/hci_request.c | 128 +++++++++++++++++++++++++++++--
> net/bluetooth/mgmt_config.c | 10 +++
> 4 files changed, 145 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 9873e1c8cd163..cfede18709d8f 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -361,6 +361,8 @@ struct hci_dev {
> __u8 ssp_debug_mode;
> __u8 hw_error_code;
> __u32 clock;
> + __u16 advmon_allowlist_duration;
> + __u16 advmon_no_filter_duration;
>
> __u16 devid_source;
> __u16 devid_vendor;
> @@ -542,6 +544,14 @@ struct hci_dev {
> struct delayed_work rpa_expired;
> bdaddr_t rpa;
>
> + enum {
> + INTERLEAVE_SCAN_NONE,
> + INTERLEAVE_SCAN_NO_FILTER,
> + INTERLEAVE_SCAN_ALLOWLIST
> + } interleave_scan_state;
> +
> + struct delayed_work interleave_scan;
> +
> #if IS_ENABLED(CONFIG_BT_LEDS)
> struct led_trigger *power_led;
> #endif
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 502552d6e9aff..65b7b74baba4c 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3592,6 +3592,10 @@ struct hci_dev *hci_alloc_dev(void)
> hdev->cur_adv_instance = 0x00;
> hdev->adv_instance_timeout = 0;
>
> + /* The default values will be chosen in the future */
this comment is misleading. You are choosing the default values right now. So just scrap it.
> + hdev->advmon_allowlist_duration = 300;
> + hdev->advmon_no_filter_duration = 500;
> +
> hdev->sniff_max_interval = 800;
> hdev->sniff_min_interval = 80;
>
> diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
> index 048d4db9d4ea5..2fd56ee21d31f 100644
> --- a/net/bluetooth/hci_request.c
> +++ b/net/bluetooth/hci_request.c
> @@ -378,6 +378,53 @@ void __hci_req_write_fast_connectable(struct hci_request *req, bool enable)
> hci_req_add(req, HCI_OP_WRITE_PAGE_SCAN_TYPE, 1, &type);
> }
>
> +static void start_interleave_scan(struct hci_dev *hdev)
> +{
> + hdev->interleave_scan_state = INTERLEAVE_SCAN_NO_FILTER;
> + queue_delayed_work(hdev->req_workqueue,
> + &hdev->interleave_scan, 0);
> +}
> +
> +static bool is_interleave_scanning(struct hci_dev *hdev)
> +{
> + return hdev->interleave_scan_state != INTERLEAVE_SCAN_NONE;
> +}
> +
> +static void cancel_interleave_scan(struct hci_dev *hdev)
> +{
> + bt_dev_dbg(hdev, "cancelling interleave scan");
> +
> + cancel_delayed_work_sync(&hdev->interleave_scan);
> +
> + hdev->interleave_scan_state = INTERLEAVE_SCAN_NONE;
> +}
> +
> +/* Return true if interleave_scan wasn't started until exiting this function,
> + * otherwise, return false
> + */
> +static bool __hci_update_interleaved_scan(struct hci_dev *hdev)
> +{
> + /* If there is at least one ADV monitors and one pending LE connection
> + * or one device to be scanned for, we should alternate between
> + * allowlist scan and one without any filters to save power.
> + */
> + bool should_interleaving = hci_is_adv_monitoring(hdev) &&
> + !(list_empty(&hdev->pend_le_conns) &&
> + list_empty(&hdev->pend_le_reports));
the name should_interleaving is not sitting right with me. Can we find a
naming that sounds a bit more like proper English. Do you actually mean
use_interleaving in this case?
> + bool is_interleaving = is_interleave_scanning(hdev);
> +
> + if (should_interleaving && !is_interleaving) {
> + start_interleave_scan(hdev);
> + bt_dev_dbg(hdev, "starting interleave scan");
> + return true;
> + }
> +
> + if (!should_interleaving && is_interleaving)
> + cancel_interleave_scan(hdev);
> +
> + return false;
> +}
> +
> /* This function controls the background scanning based on hdev->pend_le_conns
> * list. If there are pending LE connection we start the background scanning,
> * otherwise we stop it.
> @@ -450,8 +497,7 @@ static void __hci_update_background_scan(struct hci_request *req)
> hci_req_add_le_scan_disable(req, false);
>
> hci_req_add_le_passive_scan(req);
> -
> - BT_DBG("%s starting background scanning", hdev->name);
> + bt_dev_dbg(hdev, "starting background scanning");
> }
> }
>
> @@ -848,12 +894,17 @@ static u8 update_white_list(struct hci_request *req)
> return 0x00;
> }
>
> - /* Once the controller offloading of advertisement monitor is in place,
> - * the if condition should include the support of MSFT extension
> - * support. If suspend is ongoing, whitelist should be the default to
> - * prevent waking by random advertisements.
> + /* Use the allowlist unless the following conditions are all true:
> + * - We are not currently suspending
> + * - There are 1 or more ADV monitors registered
> + * - Interleaved scanning is not currently using the allowlist
> + *
> + * Once the controller offloading of advertisement monitor is in place,
> + * the above condition should include the support of MSFT extension
> + * support.
> */
> - if (!idr_is_empty(&hdev->adv_monitors_idr) && !hdev->suspended)
> + if (!idr_is_empty(&hdev->adv_monitors_idr) && !hdev->suspended &&
> + hdev->interleave_scan_state != INTERLEAVE_SCAN_ALLOWLIST)
> return 0x00;
>
> /* Select filter policy to use white list */
> @@ -1006,6 +1057,10 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
> &own_addr_type))
> return;
>
> + if (__hci_update_interleaved_scan(hdev))
> + return;
> +
> + bt_dev_dbg(hdev, "interleave state %d", hdev->interleave_scan_state);
> /* Adding or removing entries from the white list must
> * happen before enabling scanning. The controller does
> * not allow white list modification while scanning.
> @@ -1886,6 +1941,62 @@ static void adv_timeout_expire(struct work_struct *work)
> hci_dev_unlock(hdev);
> }
>
> +static int hci_req_add_le_interleaved_scan(struct hci_request *req,
> + unsigned long opt)
> +{
> + struct hci_dev *hdev = req->hdev;
> + int ret = 0;
> +
> + hci_dev_lock(hdev);
> +
> + if (hci_dev_test_flag(hdev, HCI_LE_SCAN))
> + hci_req_add_le_scan_disable(req, false);
> + hci_req_add_le_passive_scan(req);
> +
> + switch (hdev->interleave_scan_state) {
> + case INTERLEAVE_SCAN_ALLOWLIST:
> + bt_dev_dbg(hdev, "next state: allowlist");
> + hdev->interleave_scan_state = INTERLEAVE_SCAN_NO_FILTER;
> + break;
> + case INTERLEAVE_SCAN_NO_FILTER:
> + bt_dev_dbg(hdev, "next state: no filter");
> + hdev->interleave_scan_state = INTERLEAVE_SCAN_ALLOWLIST;
> + break;
> + case INTERLEAVE_SCAN_NONE:
> + BT_ERR("unexpected error");
> + ret = -1;
> + }
> +
> + hci_dev_unlock(hdev);
> +
> + return ret;
> +}
> +
> +static void interleave_scan_work(struct work_struct *work)
> +{
> + struct hci_dev *hdev = container_of(work, struct hci_dev,
> + interleave_scan.work);
> + u8 status;
> + unsigned long timeout;
> +
> + if (hdev->interleave_scan_state == INTERLEAVE_SCAN_ALLOWLIST) {
> + timeout = msecs_to_jiffies(hdev->advmon_allowlist_duration);
> + } else if (hdev->interleave_scan_state == INTERLEAVE_SCAN_NO_FILTER) {
> + timeout = msecs_to_jiffies(hdev->advmon_no_filter_duration);
> + } else {
> + bt_dev_err(hdev, "unexpected error");
> + return;
> + }
> +
> + hci_req_sync(hdev, hci_req_add_le_interleaved_scan, 0,
> + HCI_CMD_TIMEOUT, &status);
> +
> + /* Don't continue interleaving if it was canceled */
> + if (is_interleave_scanning(hdev))
> + queue_delayed_work(hdev->req_workqueue,
> + &hdev->interleave_scan, timeout);
> +}
> +
> int hci_get_random_address(struct hci_dev *hdev, bool require_privacy,
> bool use_rpa, struct adv_info *adv_instance,
> u8 *own_addr_type, bdaddr_t *rand_addr)
> @@ -3313,6 +3424,7 @@ void hci_request_setup(struct hci_dev *hdev)
> INIT_DELAYED_WORK(&hdev->le_scan_disable, le_scan_disable_work);
> INIT_DELAYED_WORK(&hdev->le_scan_restart, le_scan_restart_work);
> INIT_DELAYED_WORK(&hdev->adv_instance_expire, adv_timeout_expire);
> + INIT_DELAYED_WORK(&hdev->interleave_scan, interleave_scan_work);
> }
>
> void hci_request_cancel_all(struct hci_dev *hdev)
> @@ -3332,4 +3444,6 @@ void hci_request_cancel_all(struct hci_dev *hdev)
> cancel_delayed_work_sync(&hdev->adv_instance_expire);
> hdev->adv_instance_timeout = 0;
> }
> +
> + cancel_interleave_scan(hdev);
> }
> diff --git a/net/bluetooth/mgmt_config.c b/net/bluetooth/mgmt_config.c
> index b30b571f8caf8..2d3ad288c78ac 100644
> --- a/net/bluetooth/mgmt_config.c
> +++ b/net/bluetooth/mgmt_config.c
> @@ -67,6 +67,8 @@ int read_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
> HDEV_PARAM_U16(0x001a, le_supv_timeout),
> HDEV_PARAM_U16_JIFFIES_TO_MSECS(0x001b,
> def_le_autoconnect_timeout),
> + HDEV_PARAM_U16(0x001d, advmon_allowlist_duration),
> + HDEV_PARAM_U16(0x001e, advmon_no_filter_duration),
> };
> struct mgmt_rp_read_def_system_config *rp = (void *)params;
>
> @@ -138,6 +140,8 @@ int set_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
> case 0x0019:
> case 0x001a:
> case 0x001b:
> + case 0x001d:
> + case 0x001e:
> if (len != sizeof(u16)) {
> bt_dev_warn(hdev, "invalid length %d, exp %zu for type %d",
> len, sizeof(u16), type);
> @@ -251,6 +255,12 @@ int set_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
> hdev->def_le_autoconnect_timeout =
> msecs_to_jiffies(TLV_GET_LE16(buffer));
> break;
> + case 0x0001d:
> + hdev->advmon_allowlist_duration = TLV_GET_LE16(buffer);
> + break;
> + case 0x0001e:
> + hdev->advmon_no_filter_duration = TLV_GET_LE16(buffer);
> + break;
> default:
> bt_dev_warn(hdev, "unsupported parameter %u", type);
> break;
Regards
Marcel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v9 3/6] Bluetooth: Handle system suspend resume case
2020-11-11 7:02 [PATCH v9 1/6] Bluetooth: Replace BT_DBG with bt_dev_dbg in HCI request Howard Chung
2020-11-11 7:02 ` [PATCH v9 2/6] Bluetooth: Interleave with allowlist scan Howard Chung
@ 2020-11-11 7:02 ` Howard Chung
2020-11-11 7:02 ` [PATCH v9 4/6] Bluetooth: Handle active scan case Howard Chung
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Howard Chung @ 2020-11-11 7:02 UTC (permalink / raw)
To: linux-bluetooth, marcel, luiz.dentz
Cc: alainm, mmandlik, mcchou, Howard Chung, Abhishek Pandit-Subedi,
David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel,
netdev
This patch adds code to handle the system suspension during interleave
scan. The interleave scan will be canceled when the system is going to
sleep, and will be restarted after waking up.
Signed-off-by: Howard Chung <howardchung@google.com>
Reviewed-by: Alain Michaud <alainm@chromium.org>
Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
---
(no changes since v5)
Changes in v5:
- Remove the change in hci_req_config_le_suspend_scan
net/bluetooth/hci_request.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 2fd56ee21d31f..d943ad2885aa0 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -1293,8 +1293,10 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
hci_req_add(&req, HCI_OP_WRITE_SCAN_ENABLE, 1, &page_scan);
/* Disable LE passive scan if enabled */
- if (hci_dev_test_flag(hdev, HCI_LE_SCAN))
+ if (hci_dev_test_flag(hdev, HCI_LE_SCAN)) {
+ cancel_interleave_scan(hdev);
hci_req_add_le_scan_disable(&req, false);
+ }
/* Mark task needing completion */
set_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks);
--
2.29.2.222.g5d2a92d10f8-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v9 4/6] Bluetooth: Handle active scan case
2020-11-11 7:02 [PATCH v9 1/6] Bluetooth: Replace BT_DBG with bt_dev_dbg in HCI request Howard Chung
2020-11-11 7:02 ` [PATCH v9 2/6] Bluetooth: Interleave with allowlist scan Howard Chung
2020-11-11 7:02 ` [PATCH v9 3/6] Bluetooth: Handle system suspend resume case Howard Chung
@ 2020-11-11 7:02 ` Howard Chung
2020-11-11 7:02 ` [PATCH v9 5/6] Bluetooth: Refactor read default sys config for various types Howard Chung
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Howard Chung @ 2020-11-11 7:02 UTC (permalink / raw)
To: linux-bluetooth, marcel, luiz.dentz
Cc: alainm, mmandlik, mcchou, Howard Chung, David S. Miller,
Jakub Kicinski, Johan Hedberg, linux-kernel, netdev
This patch adds code to handle the active scan during interleave
scan. The interleave scan will be canceled when users start active scan,
and it will be restarted after active scan stopped.
Signed-off-by: Howard Chung <howardchung@google.com>
Reviewed-by: Alain Michaud <alainm@chromium.org>
Reviewed-by: Manish Mandlik <mmandlik@chromium.org>
---
(no changes since v1)
net/bluetooth/hci_request.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index d943ad2885aa0..172ccbf4f0cd2 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -3099,8 +3099,10 @@ static int active_scan(struct hci_request *req, unsigned long opt)
* running. Thus, we should temporarily stop it in order to set the
* discovery scanning parameters.
*/
- if (hci_dev_test_flag(hdev, HCI_LE_SCAN))
+ if (hci_dev_test_flag(hdev, HCI_LE_SCAN)) {
hci_req_add_le_scan_disable(req, false);
+ cancel_interleave_scan(hdev);
+ }
/* All active scans will be done with either a resolvable private
* address (when privacy feature has been enabled) or non-resolvable
--
2.29.2.222.g5d2a92d10f8-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v9 5/6] Bluetooth: Refactor read default sys config for various types
2020-11-11 7:02 [PATCH v9 1/6] Bluetooth: Replace BT_DBG with bt_dev_dbg in HCI request Howard Chung
` (2 preceding siblings ...)
2020-11-11 7:02 ` [PATCH v9 4/6] Bluetooth: Handle active scan case Howard Chung
@ 2020-11-11 7:02 ` Howard Chung
2020-11-11 11:20 ` Marcel Holtmann
2020-11-11 7:02 ` [PATCH v9 6/6] Bluetooth: Add toggle to switch off interleave scan Howard Chung
2020-11-11 11:15 ` [PATCH v9 1/6] Bluetooth: Replace BT_DBG with bt_dev_dbg in HCI request Marcel Holtmann
5 siblings, 1 reply; 10+ messages in thread
From: Howard Chung @ 2020-11-11 7:02 UTC (permalink / raw)
To: linux-bluetooth, marcel, luiz.dentz
Cc: alainm, mmandlik, mcchou, Howard Chung, David S. Miller,
Jakub Kicinski, Johan Hedberg, linux-kernel, netdev
Refactor read default system configuration function so that it's capable
of returning different types than u16
Signed-off-by: Howard Chung <howardchung@google.com>
---
(no changes since v8)
Changes in v8:
- Update the commit title and message
net/bluetooth/mgmt_config.c | 140 +++++++++++++++++++++---------------
1 file changed, 84 insertions(+), 56 deletions(-)
diff --git a/net/bluetooth/mgmt_config.c b/net/bluetooth/mgmt_config.c
index 2d3ad288c78ac..282fbf82f3192 100644
--- a/net/bluetooth/mgmt_config.c
+++ b/net/bluetooth/mgmt_config.c
@@ -11,72 +11,100 @@
#include "mgmt_util.h"
#include "mgmt_config.h"
-#define HDEV_PARAM_U16(_param_code_, _param_name_) \
-{ \
- { cpu_to_le16(_param_code_), sizeof(__u16) }, \
- { cpu_to_le16(hdev->_param_name_) } \
-}
+#define HDEV_PARAM_U16(_param_name_) \
+ struct {\
+ struct mgmt_tlv entry; \
+ __le16 value; \
+ } __packed _param_name_
-#define HDEV_PARAM_U16_JIFFIES_TO_MSECS(_param_code_, _param_name_) \
-{ \
- { cpu_to_le16(_param_code_), sizeof(__u16) }, \
- { cpu_to_le16(jiffies_to_msecs(hdev->_param_name_)) } \
-}
+#define TLV_SET_U16(_param_code_, _param_name_) \
+ { \
+ { cpu_to_le16(_param_code_), sizeof(__u16) }, \
+ cpu_to_le16(hdev->_param_name_) \
+ }
+
+#define TLV_SET_U16_JIFFIES_TO_MSECS(_param_code_, _param_name_) \
+ { \
+ { cpu_to_le16(_param_code_), sizeof(__u16) }, \
+ cpu_to_le16(jiffies_to_msecs(hdev->_param_name_)) \
+ }
int read_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
u16 data_len)
{
- struct {
- struct mgmt_tlv entry;
- union {
- /* This is a simplification for now since all values
- * are 16 bits. In the future, this code may need
- * refactoring to account for variable length values
- * and properly calculate the required buffer size.
- */
- __le16 value;
- };
- } __packed params[] = {
+ int ret;
+ struct mgmt_rp_read_def_system_config {
/* Please see mgmt-api.txt for documentation of these values */
- HDEV_PARAM_U16(0x0000, def_page_scan_type),
- HDEV_PARAM_U16(0x0001, def_page_scan_int),
- HDEV_PARAM_U16(0x0002, def_page_scan_window),
- HDEV_PARAM_U16(0x0003, def_inq_scan_type),
- HDEV_PARAM_U16(0x0004, def_inq_scan_int),
- HDEV_PARAM_U16(0x0005, def_inq_scan_window),
- HDEV_PARAM_U16(0x0006, def_br_lsto),
- HDEV_PARAM_U16(0x0007, def_page_timeout),
- HDEV_PARAM_U16(0x0008, sniff_min_interval),
- HDEV_PARAM_U16(0x0009, sniff_max_interval),
- HDEV_PARAM_U16(0x000a, le_adv_min_interval),
- HDEV_PARAM_U16(0x000b, le_adv_max_interval),
- HDEV_PARAM_U16(0x000c, def_multi_adv_rotation_duration),
- HDEV_PARAM_U16(0x000d, le_scan_interval),
- HDEV_PARAM_U16(0x000e, le_scan_window),
- HDEV_PARAM_U16(0x000f, le_scan_int_suspend),
- HDEV_PARAM_U16(0x0010, le_scan_window_suspend),
- HDEV_PARAM_U16(0x0011, le_scan_int_discovery),
- HDEV_PARAM_U16(0x0012, le_scan_window_discovery),
- HDEV_PARAM_U16(0x0013, le_scan_int_adv_monitor),
- HDEV_PARAM_U16(0x0014, le_scan_window_adv_monitor),
- HDEV_PARAM_U16(0x0015, le_scan_int_connect),
- HDEV_PARAM_U16(0x0016, le_scan_window_connect),
- HDEV_PARAM_U16(0x0017, le_conn_min_interval),
- HDEV_PARAM_U16(0x0018, le_conn_max_interval),
- HDEV_PARAM_U16(0x0019, le_conn_latency),
- HDEV_PARAM_U16(0x001a, le_supv_timeout),
- HDEV_PARAM_U16_JIFFIES_TO_MSECS(0x001b,
- def_le_autoconnect_timeout),
- HDEV_PARAM_U16(0x001d, advmon_allowlist_duration),
- HDEV_PARAM_U16(0x001e, advmon_no_filter_duration),
+ HDEV_PARAM_U16(def_page_scan_type);
+ HDEV_PARAM_U16(def_page_scan_int);
+ HDEV_PARAM_U16(def_page_scan_window);
+ HDEV_PARAM_U16(def_inq_scan_type);
+ HDEV_PARAM_U16(def_inq_scan_int);
+ HDEV_PARAM_U16(def_inq_scan_window);
+ HDEV_PARAM_U16(def_br_lsto);
+ HDEV_PARAM_U16(def_page_timeout);
+ HDEV_PARAM_U16(sniff_min_interval);
+ HDEV_PARAM_U16(sniff_max_interval);
+ HDEV_PARAM_U16(le_adv_min_interval);
+ HDEV_PARAM_U16(le_adv_max_interval);
+ HDEV_PARAM_U16(def_multi_adv_rotation_duration);
+ HDEV_PARAM_U16(le_scan_interval);
+ HDEV_PARAM_U16(le_scan_window);
+ HDEV_PARAM_U16(le_scan_int_suspend);
+ HDEV_PARAM_U16(le_scan_window_suspend);
+ HDEV_PARAM_U16(le_scan_int_discovery);
+ HDEV_PARAM_U16(le_scan_window_discovery);
+ HDEV_PARAM_U16(le_scan_int_adv_monitor);
+ HDEV_PARAM_U16(le_scan_window_adv_monitor);
+ HDEV_PARAM_U16(le_scan_int_connect);
+ HDEV_PARAM_U16(le_scan_window_connect);
+ HDEV_PARAM_U16(le_conn_min_interval);
+ HDEV_PARAM_U16(le_conn_max_interval);
+ HDEV_PARAM_U16(le_conn_latency);
+ HDEV_PARAM_U16(le_supv_timeout);
+ HDEV_PARAM_U16(def_le_autoconnect_timeout);
+ HDEV_PARAM_U16(advmon_allowlist_duration);
+ HDEV_PARAM_U16(advmon_no_filter_duration);
+ } __packed rp = {
+ TLV_SET_U16(0x0000, def_page_scan_type),
+ TLV_SET_U16(0x0001, def_page_scan_int),
+ TLV_SET_U16(0x0002, def_page_scan_window),
+ TLV_SET_U16(0x0003, def_inq_scan_type),
+ TLV_SET_U16(0x0004, def_inq_scan_int),
+ TLV_SET_U16(0x0005, def_inq_scan_window),
+ TLV_SET_U16(0x0006, def_br_lsto),
+ TLV_SET_U16(0x0007, def_page_timeout),
+ TLV_SET_U16(0x0008, sniff_min_interval),
+ TLV_SET_U16(0x0009, sniff_max_interval),
+ TLV_SET_U16(0x000a, le_adv_min_interval),
+ TLV_SET_U16(0x000b, le_adv_max_interval),
+ TLV_SET_U16(0x000c, def_multi_adv_rotation_duration),
+ TLV_SET_U16(0x000d, le_scan_interval),
+ TLV_SET_U16(0x000e, le_scan_window),
+ TLV_SET_U16(0x000f, le_scan_int_suspend),
+ TLV_SET_U16(0x0010, le_scan_window_suspend),
+ TLV_SET_U16(0x0011, le_scan_int_discovery),
+ TLV_SET_U16(0x0012, le_scan_window_discovery),
+ TLV_SET_U16(0x0013, le_scan_int_adv_monitor),
+ TLV_SET_U16(0x0014, le_scan_window_adv_monitor),
+ TLV_SET_U16(0x0015, le_scan_int_connect),
+ TLV_SET_U16(0x0016, le_scan_window_connect),
+ TLV_SET_U16(0x0017, le_conn_min_interval),
+ TLV_SET_U16(0x0018, le_conn_max_interval),
+ TLV_SET_U16(0x0019, le_conn_latency),
+ TLV_SET_U16(0x001a, le_supv_timeout),
+ TLV_SET_U16_JIFFIES_TO_MSECS(0x001b,
+ def_le_autoconnect_timeout),
+ TLV_SET_U16(0x001d, advmon_allowlist_duration),
+ TLV_SET_U16(0x001e, advmon_no_filter_duration),
};
- struct mgmt_rp_read_def_system_config *rp = (void *)params;
bt_dev_dbg(hdev, "sock %p", sk);
- return mgmt_cmd_complete(sk, hdev->id,
- MGMT_OP_READ_DEF_SYSTEM_CONFIG,
- 0, rp, sizeof(params));
+ ret = mgmt_cmd_complete(sk, hdev->id,
+ MGMT_OP_READ_DEF_SYSTEM_CONFIG,
+ 0, &rp, sizeof(rp));
+ return ret;
}
#define TO_TLV(x) ((struct mgmt_tlv *)(x))
--
2.29.2.222.g5d2a92d10f8-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v9 5/6] Bluetooth: Refactor read default sys config for various types
2020-11-11 7:02 ` [PATCH v9 5/6] Bluetooth: Refactor read default sys config for various types Howard Chung
@ 2020-11-11 11:20 ` Marcel Holtmann
2020-11-12 3:40 ` Yun-hao Chung
0 siblings, 1 reply; 10+ messages in thread
From: Marcel Holtmann @ 2020-11-11 11:20 UTC (permalink / raw)
To: Howard Chung
Cc: linux-bluetooth, Luiz Augusto von Dentz, alainm, mmandlik,
mcchou, David S. Miller, Jakub Kicinski, Johan Hedberg,
linux-kernel, netdev
Hi Howard,
> Refactor read default system configuration function so that it's capable
> of returning different types than u16
>
> Signed-off-by: Howard Chung <howardchung@google.com>
> ---
>
> (no changes since v8)
>
> Changes in v8:
> - Update the commit title and message
>
> net/bluetooth/mgmt_config.c | 140 +++++++++++++++++++++---------------
> 1 file changed, 84 insertions(+), 56 deletions(-)
>
> diff --git a/net/bluetooth/mgmt_config.c b/net/bluetooth/mgmt_config.c
> index 2d3ad288c78ac..282fbf82f3192 100644
> --- a/net/bluetooth/mgmt_config.c
> +++ b/net/bluetooth/mgmt_config.c
> @@ -11,72 +11,100 @@
> #include "mgmt_util.h"
> #include "mgmt_config.h"
>
> -#define HDEV_PARAM_U16(_param_code_, _param_name_) \
> -{ \
> - { cpu_to_le16(_param_code_), sizeof(__u16) }, \
> - { cpu_to_le16(hdev->_param_name_) } \
> -}
> +#define HDEV_PARAM_U16(_param_name_) \
> + struct {\
> + struct mgmt_tlv entry; \
> + __le16 value; \
> + } __packed _param_name_
>
> -#define HDEV_PARAM_U16_JIFFIES_TO_MSECS(_param_code_, _param_name_) \
> -{ \
> - { cpu_to_le16(_param_code_), sizeof(__u16) }, \
> - { cpu_to_le16(jiffies_to_msecs(hdev->_param_name_)) } \
> -}
> +#define TLV_SET_U16(_param_code_, _param_name_) \
> + { \
> + { cpu_to_le16(_param_code_), sizeof(__u16) }, \
> + cpu_to_le16(hdev->_param_name_) \
> + }
> +
> +#define TLV_SET_U16_JIFFIES_TO_MSECS(_param_code_, _param_name_) \
> + { \
> + { cpu_to_le16(_param_code_), sizeof(__u16) }, \
> + cpu_to_le16(jiffies_to_msecs(hdev->_param_name_)) \
> + }
>
> int read_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
> u16 data_len)
> {
> - struct {
> - struct mgmt_tlv entry;
> - union {
> - /* This is a simplification for now since all values
> - * are 16 bits. In the future, this code may need
> - * refactoring to account for variable length values
> - * and properly calculate the required buffer size.
> - */
> - __le16 value;
> - };
> - } __packed params[] = {
> + int ret;
> + struct mgmt_rp_read_def_system_config {
> /* Please see mgmt-api.txt for documentation of these values */
> - HDEV_PARAM_U16(0x0000, def_page_scan_type),
> - HDEV_PARAM_U16(0x0001, def_page_scan_int),
> - HDEV_PARAM_U16(0x0002, def_page_scan_window),
> - HDEV_PARAM_U16(0x0003, def_inq_scan_type),
> - HDEV_PARAM_U16(0x0004, def_inq_scan_int),
> - HDEV_PARAM_U16(0x0005, def_inq_scan_window),
> - HDEV_PARAM_U16(0x0006, def_br_lsto),
> - HDEV_PARAM_U16(0x0007, def_page_timeout),
> - HDEV_PARAM_U16(0x0008, sniff_min_interval),
> - HDEV_PARAM_U16(0x0009, sniff_max_interval),
> - HDEV_PARAM_U16(0x000a, le_adv_min_interval),
> - HDEV_PARAM_U16(0x000b, le_adv_max_interval),
> - HDEV_PARAM_U16(0x000c, def_multi_adv_rotation_duration),
> - HDEV_PARAM_U16(0x000d, le_scan_interval),
> - HDEV_PARAM_U16(0x000e, le_scan_window),
> - HDEV_PARAM_U16(0x000f, le_scan_int_suspend),
> - HDEV_PARAM_U16(0x0010, le_scan_window_suspend),
> - HDEV_PARAM_U16(0x0011, le_scan_int_discovery),
> - HDEV_PARAM_U16(0x0012, le_scan_window_discovery),
> - HDEV_PARAM_U16(0x0013, le_scan_int_adv_monitor),
> - HDEV_PARAM_U16(0x0014, le_scan_window_adv_monitor),
> - HDEV_PARAM_U16(0x0015, le_scan_int_connect),
> - HDEV_PARAM_U16(0x0016, le_scan_window_connect),
> - HDEV_PARAM_U16(0x0017, le_conn_min_interval),
> - HDEV_PARAM_U16(0x0018, le_conn_max_interval),
> - HDEV_PARAM_U16(0x0019, le_conn_latency),
> - HDEV_PARAM_U16(0x001a, le_supv_timeout),
> - HDEV_PARAM_U16_JIFFIES_TO_MSECS(0x001b,
> - def_le_autoconnect_timeout),
> - HDEV_PARAM_U16(0x001d, advmon_allowlist_duration),
> - HDEV_PARAM_U16(0x001e, advmon_no_filter_duration),
> + HDEV_PARAM_U16(def_page_scan_type);
> + HDEV_PARAM_U16(def_page_scan_int);
> + HDEV_PARAM_U16(def_page_scan_window);
> + HDEV_PARAM_U16(def_inq_scan_type);
> + HDEV_PARAM_U16(def_inq_scan_int);
> + HDEV_PARAM_U16(def_inq_scan_window);
> + HDEV_PARAM_U16(def_br_lsto);
> + HDEV_PARAM_U16(def_page_timeout);
> + HDEV_PARAM_U16(sniff_min_interval);
> + HDEV_PARAM_U16(sniff_max_interval);
> + HDEV_PARAM_U16(le_adv_min_interval);
> + HDEV_PARAM_U16(le_adv_max_interval);
> + HDEV_PARAM_U16(def_multi_adv_rotation_duration);
> + HDEV_PARAM_U16(le_scan_interval);
> + HDEV_PARAM_U16(le_scan_window);
> + HDEV_PARAM_U16(le_scan_int_suspend);
> + HDEV_PARAM_U16(le_scan_window_suspend);
> + HDEV_PARAM_U16(le_scan_int_discovery);
> + HDEV_PARAM_U16(le_scan_window_discovery);
> + HDEV_PARAM_U16(le_scan_int_adv_monitor);
> + HDEV_PARAM_U16(le_scan_window_adv_monitor);
> + HDEV_PARAM_U16(le_scan_int_connect);
> + HDEV_PARAM_U16(le_scan_window_connect);
> + HDEV_PARAM_U16(le_conn_min_interval);
> + HDEV_PARAM_U16(le_conn_max_interval);
> + HDEV_PARAM_U16(le_conn_latency);
> + HDEV_PARAM_U16(le_supv_timeout);
> + HDEV_PARAM_U16(def_le_autoconnect_timeout);
> + HDEV_PARAM_U16(advmon_allowlist_duration);
> + HDEV_PARAM_U16(advmon_no_filter_duration);
> + } __packed rp = {
> + TLV_SET_U16(0x0000, def_page_scan_type),
> + TLV_SET_U16(0x0001, def_page_scan_int),
> + TLV_SET_U16(0x0002, def_page_scan_window),
> + TLV_SET_U16(0x0003, def_inq_scan_type),
> + TLV_SET_U16(0x0004, def_inq_scan_int),
> + TLV_SET_U16(0x0005, def_inq_scan_window),
> + TLV_SET_U16(0x0006, def_br_lsto),
> + TLV_SET_U16(0x0007, def_page_timeout),
> + TLV_SET_U16(0x0008, sniff_min_interval),
> + TLV_SET_U16(0x0009, sniff_max_interval),
> + TLV_SET_U16(0x000a, le_adv_min_interval),
> + TLV_SET_U16(0x000b, le_adv_max_interval),
> + TLV_SET_U16(0x000c, def_multi_adv_rotation_duration),
> + TLV_SET_U16(0x000d, le_scan_interval),
> + TLV_SET_U16(0x000e, le_scan_window),
> + TLV_SET_U16(0x000f, le_scan_int_suspend),
> + TLV_SET_U16(0x0010, le_scan_window_suspend),
> + TLV_SET_U16(0x0011, le_scan_int_discovery),
> + TLV_SET_U16(0x0012, le_scan_window_discovery),
> + TLV_SET_U16(0x0013, le_scan_int_adv_monitor),
> + TLV_SET_U16(0x0014, le_scan_window_adv_monitor),
> + TLV_SET_U16(0x0015, le_scan_int_connect),
> + TLV_SET_U16(0x0016, le_scan_window_connect),
> + TLV_SET_U16(0x0017, le_conn_min_interval),
> + TLV_SET_U16(0x0018, le_conn_max_interval),
> + TLV_SET_U16(0x0019, le_conn_latency),
> + TLV_SET_U16(0x001a, le_supv_timeout),
> + TLV_SET_U16_JIFFIES_TO_MSECS(0x001b,
> + def_le_autoconnect_timeout),
> + TLV_SET_U16(0x001d, advmon_allowlist_duration),
> + TLV_SET_U16(0x001e, advmon_no_filter_duration),
> };
> - struct mgmt_rp_read_def_system_config *rp = (void *)params;
>
> bt_dev_dbg(hdev, "sock %p", sk);
>
> - return mgmt_cmd_complete(sk, hdev->id,
> - MGMT_OP_READ_DEF_SYSTEM_CONFIG,
> - 0, rp, sizeof(params));
> + ret = mgmt_cmd_complete(sk, hdev->id,
> + MGMT_OP_READ_DEF_SYSTEM_CONFIG,
> + 0, &rp, sizeof(rp));
> + return ret;
> }
>
frankly I would prefer if we do the re-factoring first and only then add new
parameters.
While I am looking at this, I am also really confused about this
JIFFIES_TO_MSEC business. We should actually not store anything in jiffies
or use it in an API. Is there a good reason to keep storing things in
jiffies internally?
Regards
Marcel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v9 5/6] Bluetooth: Refactor read default sys config for various types
2020-11-11 11:20 ` Marcel Holtmann
@ 2020-11-12 3:40 ` Yun-hao Chung
0 siblings, 0 replies; 10+ messages in thread
From: Yun-hao Chung @ 2020-11-12 3:40 UTC (permalink / raw)
To: Marcel Holtmann
Cc: linux-bluetooth, Luiz Augusto von Dentz, Alain Michaud,
Manish Mandlik, Miao-chen Chou, David S. Miller, Jakub Kicinski,
Johan Hedberg, LKML, netdev
On Wed, Nov 11, 2020 at 7:20 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Howard,
>
> > Refactor read default system configuration function so that it's capable
> > of returning different types than u16
> >
> > Signed-off-by: Howard Chung <howardchung@google.com>
> > ---
> >
> > (no changes since v8)
> >
> > Changes in v8:
> > - Update the commit title and message
> >
> > net/bluetooth/mgmt_config.c | 140 +++++++++++++++++++++---------------
> > 1 file changed, 84 insertions(+), 56 deletions(-)
> >
> > diff --git a/net/bluetooth/mgmt_config.c b/net/bluetooth/mgmt_config.c
> > index 2d3ad288c78ac..282fbf82f3192 100644
> > --- a/net/bluetooth/mgmt_config.c
> > +++ b/net/bluetooth/mgmt_config.c
> > @@ -11,72 +11,100 @@
> > #include "mgmt_util.h"
> > #include "mgmt_config.h"
> >
> > -#define HDEV_PARAM_U16(_param_code_, _param_name_) \
> > -{ \
> > - { cpu_to_le16(_param_code_), sizeof(__u16) }, \
> > - { cpu_to_le16(hdev->_param_name_) } \
> > -}
> > +#define HDEV_PARAM_U16(_param_name_) \
> > + struct {\
> > + struct mgmt_tlv entry; \
> > + __le16 value; \
> > + } __packed _param_name_
> >
> > -#define HDEV_PARAM_U16_JIFFIES_TO_MSECS(_param_code_, _param_name_) \
> > -{ \
> > - { cpu_to_le16(_param_code_), sizeof(__u16) }, \
> > - { cpu_to_le16(jiffies_to_msecs(hdev->_param_name_)) } \
> > -}
> > +#define TLV_SET_U16(_param_code_, _param_name_) \
> > + { \
> > + { cpu_to_le16(_param_code_), sizeof(__u16) }, \
> > + cpu_to_le16(hdev->_param_name_) \
> > + }
> > +
> > +#define TLV_SET_U16_JIFFIES_TO_MSECS(_param_code_, _param_name_) \
> > + { \
> > + { cpu_to_le16(_param_code_), sizeof(__u16) }, \
> > + cpu_to_le16(jiffies_to_msecs(hdev->_param_name_)) \
> > + }
> >
> > int read_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
> > u16 data_len)
> > {
> > - struct {
> > - struct mgmt_tlv entry;
> > - union {
> > - /* This is a simplification for now since all values
> > - * are 16 bits. In the future, this code may need
> > - * refactoring to account for variable length values
> > - * and properly calculate the required buffer size.
> > - */
> > - __le16 value;
> > - };
> > - } __packed params[] = {
> > + int ret;
> > + struct mgmt_rp_read_def_system_config {
> > /* Please see mgmt-api.txt for documentation of these values */
> > - HDEV_PARAM_U16(0x0000, def_page_scan_type),
> > - HDEV_PARAM_U16(0x0001, def_page_scan_int),
> > - HDEV_PARAM_U16(0x0002, def_page_scan_window),
> > - HDEV_PARAM_U16(0x0003, def_inq_scan_type),
> > - HDEV_PARAM_U16(0x0004, def_inq_scan_int),
> > - HDEV_PARAM_U16(0x0005, def_inq_scan_window),
> > - HDEV_PARAM_U16(0x0006, def_br_lsto),
> > - HDEV_PARAM_U16(0x0007, def_page_timeout),
> > - HDEV_PARAM_U16(0x0008, sniff_min_interval),
> > - HDEV_PARAM_U16(0x0009, sniff_max_interval),
> > - HDEV_PARAM_U16(0x000a, le_adv_min_interval),
> > - HDEV_PARAM_U16(0x000b, le_adv_max_interval),
> > - HDEV_PARAM_U16(0x000c, def_multi_adv_rotation_duration),
> > - HDEV_PARAM_U16(0x000d, le_scan_interval),
> > - HDEV_PARAM_U16(0x000e, le_scan_window),
> > - HDEV_PARAM_U16(0x000f, le_scan_int_suspend),
> > - HDEV_PARAM_U16(0x0010, le_scan_window_suspend),
> > - HDEV_PARAM_U16(0x0011, le_scan_int_discovery),
> > - HDEV_PARAM_U16(0x0012, le_scan_window_discovery),
> > - HDEV_PARAM_U16(0x0013, le_scan_int_adv_monitor),
> > - HDEV_PARAM_U16(0x0014, le_scan_window_adv_monitor),
> > - HDEV_PARAM_U16(0x0015, le_scan_int_connect),
> > - HDEV_PARAM_U16(0x0016, le_scan_window_connect),
> > - HDEV_PARAM_U16(0x0017, le_conn_min_interval),
> > - HDEV_PARAM_U16(0x0018, le_conn_max_interval),
> > - HDEV_PARAM_U16(0x0019, le_conn_latency),
> > - HDEV_PARAM_U16(0x001a, le_supv_timeout),
> > - HDEV_PARAM_U16_JIFFIES_TO_MSECS(0x001b,
> > - def_le_autoconnect_timeout),
> > - HDEV_PARAM_U16(0x001d, advmon_allowlist_duration),
> > - HDEV_PARAM_U16(0x001e, advmon_no_filter_duration),
> > + HDEV_PARAM_U16(def_page_scan_type);
> > + HDEV_PARAM_U16(def_page_scan_int);
> > + HDEV_PARAM_U16(def_page_scan_window);
> > + HDEV_PARAM_U16(def_inq_scan_type);
> > + HDEV_PARAM_U16(def_inq_scan_int);
> > + HDEV_PARAM_U16(def_inq_scan_window);
> > + HDEV_PARAM_U16(def_br_lsto);
> > + HDEV_PARAM_U16(def_page_timeout);
> > + HDEV_PARAM_U16(sniff_min_interval);
> > + HDEV_PARAM_U16(sniff_max_interval);
> > + HDEV_PARAM_U16(le_adv_min_interval);
> > + HDEV_PARAM_U16(le_adv_max_interval);
> > + HDEV_PARAM_U16(def_multi_adv_rotation_duration);
> > + HDEV_PARAM_U16(le_scan_interval);
> > + HDEV_PARAM_U16(le_scan_window);
> > + HDEV_PARAM_U16(le_scan_int_suspend);
> > + HDEV_PARAM_U16(le_scan_window_suspend);
> > + HDEV_PARAM_U16(le_scan_int_discovery);
> > + HDEV_PARAM_U16(le_scan_window_discovery);
> > + HDEV_PARAM_U16(le_scan_int_adv_monitor);
> > + HDEV_PARAM_U16(le_scan_window_adv_monitor);
> > + HDEV_PARAM_U16(le_scan_int_connect);
> > + HDEV_PARAM_U16(le_scan_window_connect);
> > + HDEV_PARAM_U16(le_conn_min_interval);
> > + HDEV_PARAM_U16(le_conn_max_interval);
> > + HDEV_PARAM_U16(le_conn_latency);
> > + HDEV_PARAM_U16(le_supv_timeout);
> > + HDEV_PARAM_U16(def_le_autoconnect_timeout);
> > + HDEV_PARAM_U16(advmon_allowlist_duration);
> > + HDEV_PARAM_U16(advmon_no_filter_duration);
> > + } __packed rp = {
> > + TLV_SET_U16(0x0000, def_page_scan_type),
> > + TLV_SET_U16(0x0001, def_page_scan_int),
> > + TLV_SET_U16(0x0002, def_page_scan_window),
> > + TLV_SET_U16(0x0003, def_inq_scan_type),
> > + TLV_SET_U16(0x0004, def_inq_scan_int),
> > + TLV_SET_U16(0x0005, def_inq_scan_window),
> > + TLV_SET_U16(0x0006, def_br_lsto),
> > + TLV_SET_U16(0x0007, def_page_timeout),
> > + TLV_SET_U16(0x0008, sniff_min_interval),
> > + TLV_SET_U16(0x0009, sniff_max_interval),
> > + TLV_SET_U16(0x000a, le_adv_min_interval),
> > + TLV_SET_U16(0x000b, le_adv_max_interval),
> > + TLV_SET_U16(0x000c, def_multi_adv_rotation_duration),
> > + TLV_SET_U16(0x000d, le_scan_interval),
> > + TLV_SET_U16(0x000e, le_scan_window),
> > + TLV_SET_U16(0x000f, le_scan_int_suspend),
> > + TLV_SET_U16(0x0010, le_scan_window_suspend),
> > + TLV_SET_U16(0x0011, le_scan_int_discovery),
> > + TLV_SET_U16(0x0012, le_scan_window_discovery),
> > + TLV_SET_U16(0x0013, le_scan_int_adv_monitor),
> > + TLV_SET_U16(0x0014, le_scan_window_adv_monitor),
> > + TLV_SET_U16(0x0015, le_scan_int_connect),
> > + TLV_SET_U16(0x0016, le_scan_window_connect),
> > + TLV_SET_U16(0x0017, le_conn_min_interval),
> > + TLV_SET_U16(0x0018, le_conn_max_interval),
> > + TLV_SET_U16(0x0019, le_conn_latency),
> > + TLV_SET_U16(0x001a, le_supv_timeout),
> > + TLV_SET_U16_JIFFIES_TO_MSECS(0x001b,
> > + def_le_autoconnect_timeout),
> > + TLV_SET_U16(0x001d, advmon_allowlist_duration),
> > + TLV_SET_U16(0x001e, advmon_no_filter_duration),
> > };
> > - struct mgmt_rp_read_def_system_config *rp = (void *)params;
> >
> > bt_dev_dbg(hdev, "sock %p", sk);
> >
> > - return mgmt_cmd_complete(sk, hdev->id,
> > - MGMT_OP_READ_DEF_SYSTEM_CONFIG,
> > - 0, rp, sizeof(params));
> > + ret = mgmt_cmd_complete(sk, hdev->id,
> > + MGMT_OP_READ_DEF_SYSTEM_CONFIG,
> > + 0, &rp, sizeof(rp));
> > + return ret;
> > }
> >
>
> frankly I would prefer if we do the re-factoring first and only then add new
> parameters.
Yes. This patch only does the refactor stuff and the next patch is the one that
adds the new parameter.
>
> While I am looking at this, I am also really confused about this
> JIFFIES_TO_MSEC business. We should actually not store anything in jiffies
> or use it in an API. Is there a good reason to keep storing things in
> jiffies internally?
>
I don't know about this, but it looks like it should be in a different
patch right?
> Regards
>
> Marcel
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v9 6/6] Bluetooth: Add toggle to switch off interleave scan
2020-11-11 7:02 [PATCH v9 1/6] Bluetooth: Replace BT_DBG with bt_dev_dbg in HCI request Howard Chung
` (3 preceding siblings ...)
2020-11-11 7:02 ` [PATCH v9 5/6] Bluetooth: Refactor read default sys config for various types Howard Chung
@ 2020-11-11 7:02 ` Howard Chung
2020-11-11 11:15 ` [PATCH v9 1/6] Bluetooth: Replace BT_DBG with bt_dev_dbg in HCI request Marcel Holtmann
5 siblings, 0 replies; 10+ messages in thread
From: Howard Chung @ 2020-11-11 7:02 UTC (permalink / raw)
To: linux-bluetooth, marcel, luiz.dentz
Cc: alainm, mmandlik, mcchou, Howard Chung, David S. Miller,
Jakub Kicinski, Johan Hedberg, linux-kernel, netdev
This patch add a configurable parameter to switch off the interleave
scan feature.
Reviewed-by: Alain Michaud <alainm@chromium.org>
Signed-off-by: Howard Chung <howardchung@google.com>
---
Changes in v9:
- Update and rename the macro TLV_GET_LE8
Changes in v7:
- Fix bt_dev_warn arguemnt type warning
Changes in v6:
- Set EnableAdvMonInterleaveScan to 1 byte long
Changes in v4:
- Set EnableAdvMonInterleaveScan default to Disable
- Fix 80 chars limit in mgmt_config.c
include/net/bluetooth/hci_core.h | 1 +
net/bluetooth/hci_core.c | 1 +
net/bluetooth/hci_request.c | 3 ++-
net/bluetooth/mgmt_config.c | 41 +++++++++++++++++++++++++-------
4 files changed, 37 insertions(+), 9 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index cfede18709d8f..63c6d656564a1 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -363,6 +363,7 @@ struct hci_dev {
__u32 clock;
__u16 advmon_allowlist_duration;
__u16 advmon_no_filter_duration;
+ __u8 enable_advmon_interleave_scan;
__u16 devid_source;
__u16 devid_vendor;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 65b7b74baba4c..b7cb7bfe250bd 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3595,6 +3595,7 @@ struct hci_dev *hci_alloc_dev(void)
/* The default values will be chosen in the future */
hdev->advmon_allowlist_duration = 300;
hdev->advmon_no_filter_duration = 500;
+ hdev->enable_advmon_interleave_scan = 0x00; /* Default to disable */
hdev->sniff_max_interval = 800;
hdev->sniff_min_interval = 80;
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 172ccbf4f0cd2..28520c4d2d229 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -1057,7 +1057,8 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
&own_addr_type))
return;
- if (__hci_update_interleaved_scan(hdev))
+ if (hdev->enable_advmon_interleave_scan &&
+ __hci_update_interleaved_scan(hdev))
return;
bt_dev_dbg(hdev, "interleave state %d", hdev->interleave_scan_state);
diff --git a/net/bluetooth/mgmt_config.c b/net/bluetooth/mgmt_config.c
index 282fbf82f3192..1deb0ca7a9297 100644
--- a/net/bluetooth/mgmt_config.c
+++ b/net/bluetooth/mgmt_config.c
@@ -17,12 +17,24 @@
__le16 value; \
} __packed _param_name_
+#define HDEV_PARAM_U8(_param_name_) \
+ struct {\
+ struct mgmt_tlv entry; \
+ __u8 value; \
+ } __packed _param_name_
+
#define TLV_SET_U16(_param_code_, _param_name_) \
{ \
{ cpu_to_le16(_param_code_), sizeof(__u16) }, \
cpu_to_le16(hdev->_param_name_) \
}
+#define TLV_SET_U8(_param_code_, _param_name_) \
+ { \
+ { cpu_to_le16(_param_code_), sizeof(__u8) }, \
+ hdev->_param_name_ \
+ }
+
#define TLV_SET_U16_JIFFIES_TO_MSECS(_param_code_, _param_name_) \
{ \
{ cpu_to_le16(_param_code_), sizeof(__u16) }, \
@@ -65,6 +77,7 @@ int read_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
HDEV_PARAM_U16(def_le_autoconnect_timeout);
HDEV_PARAM_U16(advmon_allowlist_duration);
HDEV_PARAM_U16(advmon_no_filter_duration);
+ HDEV_PARAM_U8(enable_advmon_interleave_scan);
} __packed rp = {
TLV_SET_U16(0x0000, def_page_scan_type),
TLV_SET_U16(0x0001, def_page_scan_int),
@@ -97,6 +110,7 @@ int read_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
def_le_autoconnect_timeout),
TLV_SET_U16(0x001d, advmon_allowlist_duration),
TLV_SET_U16(0x001e, advmon_no_filter_duration),
+ TLV_SET_U8(0x001f, enable_advmon_interleave_scan),
};
bt_dev_dbg(hdev, "sock %p", sk);
@@ -109,6 +123,7 @@ int read_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
#define TO_TLV(x) ((struct mgmt_tlv *)(x))
#define TLV_GET_LE16(tlv) le16_to_cpu(*((__le16 *)(TO_TLV(tlv)->value)))
+#define TLV_GET_U8(tlv) (*((__u8 *)(TO_TLV(tlv)->value)))
int set_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
u16 data_len)
@@ -125,6 +140,7 @@ int set_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
/* First pass to validate the tlv */
while (buffer_left >= sizeof(struct mgmt_tlv)) {
const u8 len = TO_TLV(buffer)->length;
+ size_t exp_type_len;
const u16 exp_len = sizeof(struct mgmt_tlv) +
len;
const u16 type = le16_to_cpu(TO_TLV(buffer)->type);
@@ -170,20 +186,26 @@ int set_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
case 0x001b:
case 0x001d:
case 0x001e:
- if (len != sizeof(u16)) {
- bt_dev_warn(hdev, "invalid length %d, exp %zu for type %d",
- len, sizeof(u16), type);
-
- return mgmt_cmd_status(sk, hdev->id,
- MGMT_OP_SET_DEF_SYSTEM_CONFIG,
- MGMT_STATUS_INVALID_PARAMS);
- }
+ exp_type_len = sizeof(u16);
+ break;
+ case 0x001f:
+ exp_type_len = sizeof(u8);
break;
default:
+ exp_type_len = 0;
bt_dev_warn(hdev, "unsupported parameter %u", type);
break;
}
+ if (exp_type_len && len != exp_type_len) {
+ bt_dev_warn(hdev, "invalid length %d, exp %zu for type %d",
+ len, exp_type_len, type);
+
+ return mgmt_cmd_status(sk, hdev->id,
+ MGMT_OP_SET_DEF_SYSTEM_CONFIG,
+ MGMT_STATUS_INVALID_PARAMS);
+ }
+
buffer_left -= exp_len;
buffer += exp_len;
}
@@ -289,6 +311,9 @@ int set_def_system_config(struct sock *sk, struct hci_dev *hdev, void *data,
case 0x0001e:
hdev->advmon_no_filter_duration = TLV_GET_LE16(buffer);
break;
+ case 0x0001f:
+ hdev->enable_advmon_interleave_scan = TLV_GET_U8(buffer);
+ break;
default:
bt_dev_warn(hdev, "unsupported parameter %u", type);
break;
--
2.29.2.222.g5d2a92d10f8-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v9 1/6] Bluetooth: Replace BT_DBG with bt_dev_dbg in HCI request
2020-11-11 7:02 [PATCH v9 1/6] Bluetooth: Replace BT_DBG with bt_dev_dbg in HCI request Howard Chung
` (4 preceding siblings ...)
2020-11-11 7:02 ` [PATCH v9 6/6] Bluetooth: Add toggle to switch off interleave scan Howard Chung
@ 2020-11-11 11:15 ` Marcel Holtmann
5 siblings, 0 replies; 10+ messages in thread
From: Marcel Holtmann @ 2020-11-11 11:15 UTC (permalink / raw)
To: Howard Chung
Cc: linux-bluetooth, Luiz Augusto von Dentz, Alain Michaud,
Manish Mandlik, Miao-chen Chou, David S. Miller, Jakub Kicinski,
Johan Hedberg, linux-kernel, netdev
Hi Howard,
> This replaces the BT_DBG function to bt_dev_dbg as it is cleaner to show
> the controller index in the debug message.
>
> Signed-off-by: Howard Chung <howardchung@google.com>
> ---
>
> Changes in v9:
> - Fix compile warning on patch 6/6
>
> Changes in v8:
> - Simplified logic in __hci_update_interleaved_scan
> - Remove hdev->name when calling bt_dev_dbg
> - Remove 'default' in hci_req_add_le_interleaved_scan switch block
> - Remove {} around :1915
> - Update commit message and title in v7 4/5
> - Add a cleanup patch for replacing BT_DBG with bt_dev_dbg
>
> Changes in v7:
> - Fix bt_dev_warn argument type warning
>
> Changes in v6:
> - Set parameter EnableAdvMonInterleaveScan to 1 byte long
>
> Changes in v5:
> - Rename 'adv_monitor' from many functions/variables
> - Move __hci_update_interleaved_scan into hci_req_add_le_passive_scan
> - Update the logic of update_adv_monitor_scan_state
>
> Changes in v4:
> - Rebase to bluetooth-next/master (previous 2 patches are applied)
> - Fix over 80 chars limit in mgmt_config.c
> - Set EnableAdvMonInterleaveScan default to Disable
>
> Changes in v3:
> - Remove 'Bluez' prefix
>
> Changes in v2:
> - remove 'case 0x001c' in mgmt_config.c
>
> net/bluetooth/hci_request.c | 52 ++++++++++++++++++-------------------
> 1 file changed, 26 insertions(+), 26 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply [flat|nested] 10+ messages in thread