| Message ID | 20201111150115.v9.2.Ib75f58e90c477f9b82c5598f00c59f0e95a1a352@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v9,1/6] Bluetooth: Replace BT_DBG with bt_dev_dbg in HCI request | expand |
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
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;