Hi Luiz and Marcel,
This is a quality of life improvement for the behavior of audio devices
during system suspend. This depends on a kernel change that emits
suspend/resume events:
https://patchwork.kernel.org/project/bluetooth/list/?series=325771
Right now, audio devices will be disconnected as part of suspend but
won't be reconnected when the system resumes without user interaction.
This is annoying to some users as it causes an interruption to their
normal work flow.
This change reconnects audio devices that were disconnected for suspend
using the following logic:
* In the device disconnect callback, mark any devices with the A2DP
service uuid for reconnect. The reconnect will not be queued until
resume.
* In the controller resume callback, queue any policy items that are
marked to reconnect on resume for connection with the ResumeDelay
value (default = 2s).
A reconnect is queued after the controller resumes and the delay
between resume and reconnect is configurable via the ResumeDelay key in
the Policy settings. The 2s delay was chosen arbitrarily and I think
anywhere up to 10s is probably ok. A longer delay is better to account
for spurious wakeups and Wi-Fi reconnection time (avoiding any co-ex
issues) at the downside of reconnection speed.
Here are the tests I have done with this:
- Single suspend and verified the headphones reconnect
- Suspend stress test for 25 iterations and verify both Wi-Fi and
Bluetooth audio reconnect on resume. (Ran with wake minimum time of
10s)
- Suspend test with wake time = 1s to verify that BT reconnect isn't
attempted. Ran 5 iterations with low wake time and then let it stay
awake to confirm reconnect finally completed on last resume.
- Suspend test with wake time between 1s - 4s. Ran with 5 iterations and
verified it connected several times in the middle and finally at the
end.
I've tested this on a Pixelbook Go (AC-9260 controller) and HP
Chromebook 14a (RTL8822CE controller) with GID6B headset.
I've also tested this with the Pixel Buds 2. These earbuds actually
reconnect automatically to the Chromebook (even without this policy
change) and I verified that the new changes don't break the reconnection
mechanism.
Thanks
Abhishek
Changes in v4:
- Set reconnect timer in disconnect if resume events aren't supported
- Only set reconnect timer if adapter matches current notification
- Refactor changes in src/adapter to its own commit
- Refactor enabling A2DP_SINK_UUID into its own commit
Changes in v3:
- Refactored resume notification to use btd_adapter_driver
- Renamed ReconnectAudioDelay to ResumeDelay and set default to 2
- Added A2DP_SINK_UUID to default reconnect list
Changes in v2:
- Refactored to use policy instead of connecting directly in adapter
Abhishek Pandit-Subedi (4):
adapter: Refactor kernel feature globals
adapter: Handle controller resume and notify drivers
policy: Enable reconnect for a2dp-sink in defaults
policy: Reconnect audio on controller resume
plugins/policy.c | 87 +++++++++++++++++++++++++++++++++-------
src/adapter.c | 102 +++++++++++++++++++++++++++++++++--------------
src/adapter.h | 11 +++++
src/main.c | 1 +
src/main.conf | 11 ++++-
5 files changed, 168 insertions(+), 44 deletions(-)
--
2.28.0.618.gf4bc123cb7-goog
Register for controller resume notification and notify the adapter
drivers when it occurs. Also adds the resume event kernel feature to
make sure the kernel supports this event.
---
Changes in v4: None
Changes in v3: None
Changes in v2: None
src/adapter.c | 43 +++++++++++++++++++++++++++++++++++++++++++
src/adapter.h | 2 ++
2 files changed, 45 insertions(+)
diff --git a/src/adapter.c b/src/adapter.c
index e700a78d5..d77d83d11 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -8771,6 +8771,33 @@ static void connected_callback(uint16_t index, uint16_t length,
eir_data_free(&eir_data);
}
+static void controller_resume_notify(struct btd_adapter *adapter)
+{
+ GSList *l;
+
+ for (l = adapter->drivers; l; l = g_slist_next(l)) {
+ struct btd_adapter_driver *driver = l->data;
+ if (driver->resume)
+ driver->resume(adapter);
+ }
+}
+
+static void controller_resume_callback(uint16_t index, uint16_t length,
+ const void *param, void *user_data)
+{
+ const struct mgmt_ev_controller_resume *ev = param;
+ struct btd_adapter *adapter = user_data;
+
+ if (length < sizeof(*ev)) {
+ btd_error(adapter->dev_id, "Too small device resume event");
+ return;
+ }
+
+ info("Controller resume with wake event 0x%x", ev->wake_reason);
+
+ controller_resume_notify(adapter);
+}
+
static void device_blocked_callback(uint16_t index, uint16_t length,
const void *param, void *user_data)
{
@@ -9394,6 +9421,11 @@ static void read_info_complete(uint8_t status, uint16_t length,
user_passkey_notify_callback,
adapter, NULL);
+ mgmt_register(adapter->mgmt, MGMT_EV_CONTROLLER_RESUME,
+ adapter->dev_id,
+ controller_resume_callback,
+ adapter, NULL);
+
set_dev_class(adapter);
set_name(adapter, btd_adapter_get_name(adapter));
@@ -9604,6 +9636,17 @@ static void read_commands_complete(uint8_t status, uint16_t length,
break;
}
}
+
+ for (i = 0; i < num_events; i++) {
+ uint16_t ev = get_le16(rp->opcodes + num_commands + i);
+
+ switch(ev) {
+ case MGMT_EV_CONTROLLER_RESUME:
+ DBG("kernel supports suspend/resume events");
+ kernel_features |= KERNEL_HAS_RESUME_EVT;
+ break;
+ }
+ }
}
static void read_version_complete(uint8_t status, uint16_t length,
diff --git a/src/adapter.h b/src/adapter.h
index b0ed4915f..fae2e9d3d 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -113,6 +113,7 @@ struct btd_adapter_driver {
const char *name;
int (*probe) (struct btd_adapter *adapter);
void (*remove) (struct btd_adapter *adapter);
+ void (*resume) (struct btd_adapter *adapter);
};
typedef void (*service_auth_cb) (DBusError *derr, void *user_data);
@@ -239,6 +240,7 @@ enum kernel_features {
KERNEL_BLOCKED_KEYS_SUPPORTED = 1 << 1,
KERNEL_SET_SYSTEM_CONFIG = 1 << 2,
KERNEL_EXP_FEATURES = 1 << 3,
+ KERNEL_HAS_RESUME_EVT = 1 << 4,
};
bool has_kernel_features(uint32_t feature);
--
2.28.0.618.gf4bc123cb7-goog
Move all the kernel specific feature globals into a single
kernel_features bitfield and replace all uses with the bitfield instead.
---
Changes in v4: None
Changes in v3: None
Changes in v2: None
src/adapter.c | 59 ++++++++++++++++++++++++++-------------------------
src/adapter.h | 9 ++++++++
2 files changed, 39 insertions(+), 29 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 1435e2bd7..e700a78d5 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -116,13 +116,7 @@ static const struct mgmt_blocked_key_info blocked_keys[] = {
static DBusConnection *dbus_conn = NULL;
-static bool kernel_conn_control = false;
-
-static bool kernel_blocked_keys_supported = false;
-
-static bool kernel_set_system_config = false;
-
-static bool kernel_exp_features = false;
+static uint32_t kernel_features = 0;
static GList *adapter_list = NULL;
static unsigned int adapter_remaining = 0;
@@ -678,7 +672,7 @@ static bool set_discoverable(struct btd_adapter *adapter, uint8_t mode,
DBG("sending set mode command for index %u", adapter->dev_id);
- if (kernel_conn_control) {
+ if (has_kernel_features(KERNEL_CONN_CONTROL)) {
if (mode)
set_mode(adapter, MGMT_OP_SET_CONNECTABLE, mode);
else
@@ -1334,7 +1328,7 @@ static void trigger_passive_scanning(struct btd_adapter *adapter)
* no need to start any discovery. The kernel will keep scanning
* as long as devices are in its auto-connection list.
*/
- if (kernel_conn_control)
+ if (has_kernel_features(KERNEL_CONN_CONTROL))
return;
/*
@@ -1385,7 +1379,7 @@ static void stop_passive_scanning_complete(uint8_t status, uint16_t length,
* no need to stop any discovery. The kernel will handle the
* auto-connection by itself.
*/
- if (kernel_conn_control)
+ if (has_kernel_features(KERNEL_CONN_CONTROL))
return;
/*
@@ -2816,7 +2810,7 @@ static void property_set_mode_complete(uint8_t status, uint16_t length,
static void clear_discoverable(struct btd_adapter *adapter)
{
- if (!kernel_conn_control)
+ if (!has_kernel_features(KERNEL_CONN_CONTROL))
return;
if (!(adapter->current_settings & MGMT_SETTING_DISCOVERABLE))
@@ -2876,7 +2870,7 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
break;
case MGMT_SETTING_DISCOVERABLE:
- if (kernel_conn_control) {
+ if (has_kernel_features(KERNEL_CONN_CONTROL)) {
if (mode) {
set_mode(adapter, MGMT_OP_SET_CONNECTABLE,
mode);
@@ -4193,7 +4187,8 @@ static void load_default_system_params(struct btd_adapter *adapter)
size_t len = 0;
unsigned int err;
- if (!main_opts.default_params.num_entries || !kernel_set_system_config)
+ if (!main_opts.default_params.num_entries ||
+ !has_kernel_features(KERNEL_SET_SYSTEM_CONFIG))
return;
params = malloc0(sizeof(*params) *
@@ -4878,7 +4873,7 @@ int adapter_connect_list_add(struct btd_adapter *adapter,
* adapter_auto_connect_add() function is used to maintain what to
* connect.
*/
- if (kernel_conn_control)
+ if (has_kernel_features(KERNEL_CONN_CONTROL))
return 0;
if (g_slist_find(adapter->connect_list, device)) {
@@ -4918,7 +4913,7 @@ void adapter_connect_list_remove(struct btd_adapter *adapter,
if (device == adapter->connect_le)
adapter->connect_le = NULL;
- if (kernel_conn_control)
+ if (has_kernel_features(KERNEL_CONN_CONTROL))
return;
if (!g_slist_find(adapter->connect_list, device)) {
@@ -4980,7 +4975,7 @@ void adapter_whitelist_add(struct btd_adapter *adapter, struct btd_device *dev)
{
struct mgmt_cp_add_device cp;
- if (!kernel_conn_control)
+ if (!has_kernel_features(KERNEL_CONN_CONTROL))
return;
memset(&cp, 0, sizeof(cp));
@@ -5019,7 +5014,7 @@ void adapter_whitelist_remove(struct btd_adapter *adapter, struct btd_device *de
{
struct mgmt_cp_remove_device cp;
- if (!kernel_conn_control)
+ if (!has_kernel_features(KERNEL_CONN_CONTROL))
return;
memset(&cp, 0, sizeof(cp));
@@ -5075,7 +5070,7 @@ void adapter_auto_connect_add(struct btd_adapter *adapter,
uint8_t bdaddr_type;
unsigned int id;
- if (!kernel_conn_control)
+ if (!has_kernel_features(KERNEL_CONN_CONTROL))
return;
if (g_slist_find(adapter->connect_list, device)) {
@@ -5147,7 +5142,7 @@ void adapter_set_device_wakeable(struct btd_adapter *adapter,
const bdaddr_t *bdaddr;
uint8_t bdaddr_type;
- if (!kernel_conn_control)
+ if (!has_kernel_features(KERNEL_CONN_CONTROL))
return;
bdaddr = device_get_address(device);
@@ -5224,7 +5219,7 @@ void adapter_auto_connect_remove(struct btd_adapter *adapter,
uint8_t bdaddr_type;
unsigned int id;
- if (!kernel_conn_control)
+ if (!has_kernel_features(KERNEL_CONN_CONTROL))
return;
if (!g_slist_find(adapter->connect_list, device)) {
@@ -6764,7 +6759,7 @@ connect_le:
* If kernel background scan is used then the kernel is
* responsible for connecting.
*/
- if (kernel_conn_control)
+ if (has_kernel_features(KERNEL_CONN_CONTROL))
return;
/*
@@ -8964,7 +8959,7 @@ static int clear_devices(struct btd_adapter *adapter)
{
struct mgmt_cp_remove_device cp;
- if (!kernel_conn_control)
+ if (!has_kernel_features(KERNEL_CONN_CONTROL))
return 0;
memset(&cp, 0, sizeof(cp));
@@ -9282,7 +9277,7 @@ static void read_info_complete(uint8_t status, uint16_t length,
(missing_settings & MGMT_SETTING_FAST_CONNECTABLE))
set_mode(adapter, MGMT_OP_SET_FAST_CONNECTABLE, 0x01);
- if (kernel_exp_features)
+ if (has_kernel_features(KERNEL_EXP_FEATURES))
read_exp_features(adapter);
err = adapter_register(adapter);
@@ -9403,7 +9398,8 @@ static void read_info_complete(uint8_t status, uint16_t length,
set_name(adapter, btd_adapter_get_name(adapter));
- if (kernel_blocked_keys_supported && !set_blocked_keys(adapter)) {
+ if (has_kernel_features(KERNEL_BLOCKED_KEYS_SUPPORTED) &&
+ !set_blocked_keys(adapter)) {
btd_error(adapter->dev_id,
"Failed to set blocked keys for index %u",
adapter->dev_id);
@@ -9414,7 +9410,7 @@ static void read_info_complete(uint8_t status, uint16_t length,
!(adapter->current_settings & MGMT_SETTING_BONDABLE))
set_mode(adapter, MGMT_OP_SET_BONDABLE, 0x01);
- if (!kernel_conn_control)
+ if (!has_kernel_features(KERNEL_CONN_CONTROL))
set_mode(adapter, MGMT_OP_SET_CONNECTABLE, 0x01);
else if (adapter->current_settings & MGMT_SETTING_CONNECTABLE)
set_mode(adapter, MGMT_OP_SET_CONNECTABLE, 0x00);
@@ -9590,19 +9586,19 @@ static void read_commands_complete(uint8_t status, uint16_t length,
switch (op) {
case MGMT_OP_ADD_DEVICE:
DBG("enabling kernel-side connection control");
- kernel_conn_control = true;
+ kernel_features |= KERNEL_CONN_CONTROL;
break;
case MGMT_OP_SET_BLOCKED_KEYS:
DBG("kernel supports the set_blocked_keys op");
- kernel_blocked_keys_supported = true;
+ kernel_features |= KERNEL_BLOCKED_KEYS_SUPPORTED;
break;
case MGMT_OP_SET_DEF_SYSTEM_CONFIG:
DBG("kernel supports set system confic");
- kernel_set_system_config = true;
+ kernel_features |= KERNEL_SET_SYSTEM_CONFIG;
break;
case MGMT_OP_READ_EXP_FEATURES_INFO:
DBG("kernel supports exp features");
- kernel_exp_features = true;
+ kernel_features |= KERNEL_EXP_FEATURES;
break;
default:
break;
@@ -9768,3 +9764,8 @@ bool btd_le_connect_before_pairing(void)
return false;
}
+
+bool has_kernel_features(uint32_t features)
+{
+ return !!(kernel_features & features);
+}
diff --git a/src/adapter.h b/src/adapter.h
index f8ac20261..b0ed4915f 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -233,3 +233,12 @@ void btd_adapter_for_each_device(struct btd_adapter *adapter,
void *data);
bool btd_le_connect_before_pairing(void);
+
+enum kernel_features {
+ KERNEL_CONN_CONTROL = 1 << 0,
+ KERNEL_BLOCKED_KEYS_SUPPORTED = 1 << 1,
+ KERNEL_SET_SYSTEM_CONFIG = 1 << 2,
+ KERNEL_EXP_FEATURES = 1 << 3,
+};
+
+bool has_kernel_features(uint32_t feature);
--
2.28.0.618.gf4bc123cb7-goog
During system suspend, all peer devices are disconnected. On resume, HID
devices will reconnect but audio devices stay disconnected. As a quality
of life improvement, mark audio devices that were disconnected due to
suspend and attempt to reconnect them when the controller resumes (after
a delay for better co-existence with Wi-Fi).
---
Changes in v4:
- Set reconnect timer in disconnect if resume events aren't supported
- Only set reconnect timer if adapter matches current notification
- Refactor changes in src/adapter to its own commit
- Refactor enabling A2DP_SINK_UUID into its own commit
Changes in v3:
- Refactored resume notification to use btd_adapter_driver
- Renamed ReconnectAudioDelay to ResumeDelay and set default to 2
- Added A2DP_SINK_UUID to default reconnect list
Changes in v2:
- Refactored to use policy instead of connecting directly in adapter
plugins/policy.c | 84 ++++++++++++++++++++++++++++++++++++++++--------
src/main.c | 1 +
src/main.conf | 9 ++++++
3 files changed, 81 insertions(+), 13 deletions(-)
diff --git a/plugins/policy.c b/plugins/policy.c
index c18ca8d1f..6bd389518 100644
--- a/plugins/policy.c
+++ b/plugins/policy.c
@@ -62,6 +62,7 @@ struct reconnect_data {
guint timer;
bool active;
unsigned int attempt;
+ bool on_resume;
};
static const char *default_reconnect[] = {
@@ -76,6 +77,9 @@ static const int default_intervals[] = { 1, 2, 4, 8, 16, 32, 64 };
static int *reconnect_intervals = NULL;
static size_t reconnect_intervals_len = 0;
+static const int default_resume_delay = 2;
+static int resume_delay;
+
static GSList *reconnects = NULL;
static unsigned int service_id = 0;
@@ -712,6 +716,9 @@ static gboolean reconnect_timeout(gpointer data)
/* Mark the GSource as invalid */
reconnect->timer = 0;
+ /* Mark any reconnect on resume as handled */
+ reconnect->on_resume = false;
+
err = btd_device_connect_services(reconnect->dev, reconnect->services);
if (err < 0) {
error("Reconnecting services failed: %s (%d)",
@@ -725,14 +732,17 @@ static gboolean reconnect_timeout(gpointer data)
return FALSE;
}
-static void reconnect_set_timer(struct reconnect_data *reconnect)
+static void reconnect_set_timer(struct reconnect_data *reconnect, int timeout)
{
- static int timeout = 0;
+ static int interval_timeout = 0;
reconnect->active = true;
if (reconnect->attempt < reconnect_intervals_len)
- timeout = reconnect_intervals[reconnect->attempt];
+ interval_timeout = reconnect_intervals[reconnect->attempt];
+
+ if (timeout < 0)
+ timeout = interval_timeout;
DBG("attempt %u/%zu %d seconds", reconnect->attempt + 1,
reconnect_attempts, timeout);
@@ -744,10 +754,14 @@ static void reconnect_set_timer(struct reconnect_data *reconnect)
static void disconnect_cb(struct btd_device *dev, uint8_t reason)
{
struct reconnect_data *reconnect;
+ struct btd_service *service;
+ struct policy_data *data;
DBG("reason %u", reason);
- if (reason != MGMT_DEV_DISCONN_TIMEOUT)
+ /* Only attempt reconnect for the following reasons */
+ if (reason != MGMT_DEV_DISCONN_TIMEOUT &&
+ reason != MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND)
return;
reconnect = reconnect_find(dev);
@@ -756,10 +770,47 @@ static void disconnect_cb(struct btd_device *dev, uint8_t reason)
reconnect_reset(reconnect);
- DBG("Device %s identified for auto-reconnection",
- device_get_path(dev));
+ DBG("Device %s identified for auto-reconnection", device_get_path(dev));
+
+ switch(reason)
+ {
+ case MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND:
+ if (btd_device_get_service(dev, A2DP_SINK_UUID)) {
+ DBG("%s configured to reconnect on resume",
+ device_get_path(dev));
- reconnect_set_timer(reconnect);
+ reconnect->on_resume = true;
+
+ /* If the kernel supports resume events, it is
+ * preferable to set the reconnect timer there as it is
+ * a more predictable delay.
+ */
+ if (!has_kernel_features(KERNEL_HAS_RESUME_EVT))
+ reconnect_set_timer(reconnect, resume_delay);
+ }
+ break;
+ case MGMT_DEV_DISCONN_TIMEOUT:
+ reconnect_set_timer(reconnect, -1);
+ break;
+ default:
+ DBG("Developer error. Reason = %d", reason);
+ break;
+ }
+}
+
+static void policy_adapter_resume(struct btd_adapter *adapter)
+{
+ GSList *l;
+
+ /* Check if devices on this adapter need to be reconnected on resume */
+ for (l = reconnects; l; l = g_slist_next(l)) {
+ struct reconnect_data *reconnect = l->data;
+
+ if (reconnect->on_resume &&
+ device_get_adapter(reconnect->dev) == adapter) {
+ reconnect_set_timer(reconnect, resume_delay);
+ }
+ }
}
static void conn_fail_cb(struct btd_device *dev, uint8_t status)
@@ -787,14 +838,15 @@ static void conn_fail_cb(struct btd_device *dev, uint8_t status)
return;
}
- reconnect_set_timer(reconnect);
+ reconnect_set_timer(reconnect, -1);
}
static int policy_adapter_probe(struct btd_adapter *adapter)
{
DBG("");
- btd_adapter_restore_powered(adapter);
+ if (auto_enable)
+ btd_adapter_restore_powered(adapter);
return 0;
}
@@ -802,6 +854,7 @@ static int policy_adapter_probe(struct btd_adapter *adapter)
static struct btd_adapter_driver policy_driver = {
.name = "policy",
.probe = policy_adapter_probe,
+ .resume = policy_adapter_resume,
};
static int policy_init(void)
@@ -855,14 +908,20 @@ static int policy_init(void)
auto_enable = g_key_file_get_boolean(conf, "Policy", "AutoEnable",
NULL);
+ resume_delay = g_key_file_get_integer(
+ conf, "Policy", "ResumeDelay", &gerr);
+
+ if (gerr) {
+ g_clear_error(&gerr);
+ resume_delay = default_resume_delay;
+ }
done:
if (reconnect_uuids && reconnect_uuids[0] && reconnect_attempts) {
btd_add_disconnect_cb(disconnect_cb);
btd_add_conn_fail_cb(conn_fail_cb);
}
- if (auto_enable)
- btd_register_adapter_driver(&policy_driver);
+ btd_register_adapter_driver(&policy_driver);
return 0;
}
@@ -883,8 +942,7 @@ static void policy_exit(void)
btd_service_remove_state_cb(service_id);
- if (auto_enable)
- btd_unregister_adapter_driver(&policy_driver);
+ btd_unregister_adapter_driver(&policy_driver);
}
BLUETOOTH_PLUGIN_DEFINE(policy, VERSION, BLUETOOTH_PLUGIN_PRIORITY_DEFAULT,
diff --git a/src/main.c b/src/main.c
index b37c32948..038f867b5 100644
--- a/src/main.c
+++ b/src/main.c
@@ -131,6 +131,7 @@ static const char *policy_options[] = {
"ReconnectAttempts",
"ReconnectIntervals",
"AutoEnable",
+ "ResumeDelay",
NULL
};
diff --git a/src/main.conf b/src/main.conf
index e1d77cc47..9f882e65a 100644
--- a/src/main.conf
+++ b/src/main.conf
@@ -202,3 +202,12 @@
# This includes adapters present on start as well as adapters that are plugged
# in later on. Defaults to 'false'.
#AutoEnable=false
+
+# Audio devices that were disconnected due to suspend will be reconnected on
+# resume. ResumeDelay determines the delay between when the controller
+# resumes from suspend and a connection attempt is made. A longer delay is
+# better for better co-existence with Wi-Fi.
+# The value is in seconds.
+# Default: 2
+#ResumeDelay = 2
+
--
2.28.0.618.gf4bc123cb7-goog
Hi Abhishek,
On Fri, Sep 11, 2020 at 3:30 PM Abhishek Pandit-Subedi
<[email protected]> wrote:
>
> Move all the kernel specific feature globals into a single
> kernel_features bitfield and replace all uses with the bitfield instead.
> ---
>
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
> src/adapter.c | 59 ++++++++++++++++++++++++++-------------------------
> src/adapter.h | 9 ++++++++
> 2 files changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 1435e2bd7..e700a78d5 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -116,13 +116,7 @@ static const struct mgmt_blocked_key_info blocked_keys[] = {
>
> static DBusConnection *dbus_conn = NULL;
>
> -static bool kernel_conn_control = false;
> -
> -static bool kernel_blocked_keys_supported = false;
> -
> -static bool kernel_set_system_config = false;
> -
> -static bool kernel_exp_features = false;
> +static uint32_t kernel_features = 0;
>
> static GList *adapter_list = NULL;
> static unsigned int adapter_remaining = 0;
> @@ -678,7 +672,7 @@ static bool set_discoverable(struct btd_adapter *adapter, uint8_t mode,
>
> DBG("sending set mode command for index %u", adapter->dev_id);
>
> - if (kernel_conn_control) {
> + if (has_kernel_features(KERNEL_CONN_CONTROL)) {
> if (mode)
> set_mode(adapter, MGMT_OP_SET_CONNECTABLE, mode);
> else
> @@ -1334,7 +1328,7 @@ static void trigger_passive_scanning(struct btd_adapter *adapter)
> * no need to start any discovery. The kernel will keep scanning
> * as long as devices are in its auto-connection list.
> */
> - if (kernel_conn_control)
> + if (has_kernel_features(KERNEL_CONN_CONTROL))
> return;
>
> /*
> @@ -1385,7 +1379,7 @@ static void stop_passive_scanning_complete(uint8_t status, uint16_t length,
> * no need to stop any discovery. The kernel will handle the
> * auto-connection by itself.
> */
> - if (kernel_conn_control)
> + if (has_kernel_features(KERNEL_CONN_CONTROL))
> return;
>
> /*
> @@ -2816,7 +2810,7 @@ static void property_set_mode_complete(uint8_t status, uint16_t length,
>
> static void clear_discoverable(struct btd_adapter *adapter)
> {
> - if (!kernel_conn_control)
> + if (!has_kernel_features(KERNEL_CONN_CONTROL))
> return;
>
> if (!(adapter->current_settings & MGMT_SETTING_DISCOVERABLE))
> @@ -2876,7 +2870,7 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
>
> break;
> case MGMT_SETTING_DISCOVERABLE:
> - if (kernel_conn_control) {
> + if (has_kernel_features(KERNEL_CONN_CONTROL)) {
> if (mode) {
> set_mode(adapter, MGMT_OP_SET_CONNECTABLE,
> mode);
> @@ -4193,7 +4187,8 @@ static void load_default_system_params(struct btd_adapter *adapter)
> size_t len = 0;
> unsigned int err;
>
> - if (!main_opts.default_params.num_entries || !kernel_set_system_config)
> + if (!main_opts.default_params.num_entries ||
> + !has_kernel_features(KERNEL_SET_SYSTEM_CONFIG))
> return;
>
> params = malloc0(sizeof(*params) *
> @@ -4878,7 +4873,7 @@ int adapter_connect_list_add(struct btd_adapter *adapter,
> * adapter_auto_connect_add() function is used to maintain what to
> * connect.
> */
> - if (kernel_conn_control)
> + if (has_kernel_features(KERNEL_CONN_CONTROL))
> return 0;
>
> if (g_slist_find(adapter->connect_list, device)) {
> @@ -4918,7 +4913,7 @@ void adapter_connect_list_remove(struct btd_adapter *adapter,
> if (device == adapter->connect_le)
> adapter->connect_le = NULL;
>
> - if (kernel_conn_control)
> + if (has_kernel_features(KERNEL_CONN_CONTROL))
> return;
>
> if (!g_slist_find(adapter->connect_list, device)) {
> @@ -4980,7 +4975,7 @@ void adapter_whitelist_add(struct btd_adapter *adapter, struct btd_device *dev)
> {
> struct mgmt_cp_add_device cp;
>
> - if (!kernel_conn_control)
> + if (!has_kernel_features(KERNEL_CONN_CONTROL))
> return;
>
> memset(&cp, 0, sizeof(cp));
> @@ -5019,7 +5014,7 @@ void adapter_whitelist_remove(struct btd_adapter *adapter, struct btd_device *de
> {
> struct mgmt_cp_remove_device cp;
>
> - if (!kernel_conn_control)
> + if (!has_kernel_features(KERNEL_CONN_CONTROL))
> return;
>
> memset(&cp, 0, sizeof(cp));
> @@ -5075,7 +5070,7 @@ void adapter_auto_connect_add(struct btd_adapter *adapter,
> uint8_t bdaddr_type;
> unsigned int id;
>
> - if (!kernel_conn_control)
> + if (!has_kernel_features(KERNEL_CONN_CONTROL))
> return;
>
> if (g_slist_find(adapter->connect_list, device)) {
> @@ -5147,7 +5142,7 @@ void adapter_set_device_wakeable(struct btd_adapter *adapter,
> const bdaddr_t *bdaddr;
> uint8_t bdaddr_type;
>
> - if (!kernel_conn_control)
> + if (!has_kernel_features(KERNEL_CONN_CONTROL))
> return;
>
> bdaddr = device_get_address(device);
> @@ -5224,7 +5219,7 @@ void adapter_auto_connect_remove(struct btd_adapter *adapter,
> uint8_t bdaddr_type;
> unsigned int id;
>
> - if (!kernel_conn_control)
> + if (!has_kernel_features(KERNEL_CONN_CONTROL))
> return;
>
> if (!g_slist_find(adapter->connect_list, device)) {
> @@ -6764,7 +6759,7 @@ connect_le:
> * If kernel background scan is used then the kernel is
> * responsible for connecting.
> */
> - if (kernel_conn_control)
> + if (has_kernel_features(KERNEL_CONN_CONTROL))
> return;
>
> /*
> @@ -8964,7 +8959,7 @@ static int clear_devices(struct btd_adapter *adapter)
> {
> struct mgmt_cp_remove_device cp;
>
> - if (!kernel_conn_control)
> + if (!has_kernel_features(KERNEL_CONN_CONTROL))
> return 0;
>
> memset(&cp, 0, sizeof(cp));
> @@ -9282,7 +9277,7 @@ static void read_info_complete(uint8_t status, uint16_t length,
> (missing_settings & MGMT_SETTING_FAST_CONNECTABLE))
> set_mode(adapter, MGMT_OP_SET_FAST_CONNECTABLE, 0x01);
>
> - if (kernel_exp_features)
> + if (has_kernel_features(KERNEL_EXP_FEATURES))
> read_exp_features(adapter);
>
> err = adapter_register(adapter);
> @@ -9403,7 +9398,8 @@ static void read_info_complete(uint8_t status, uint16_t length,
>
> set_name(adapter, btd_adapter_get_name(adapter));
>
> - if (kernel_blocked_keys_supported && !set_blocked_keys(adapter)) {
> + if (has_kernel_features(KERNEL_BLOCKED_KEYS_SUPPORTED) &&
> + !set_blocked_keys(adapter)) {
> btd_error(adapter->dev_id,
> "Failed to set blocked keys for index %u",
> adapter->dev_id);
> @@ -9414,7 +9410,7 @@ static void read_info_complete(uint8_t status, uint16_t length,
> !(adapter->current_settings & MGMT_SETTING_BONDABLE))
> set_mode(adapter, MGMT_OP_SET_BONDABLE, 0x01);
>
> - if (!kernel_conn_control)
> + if (!has_kernel_features(KERNEL_CONN_CONTROL))
> set_mode(adapter, MGMT_OP_SET_CONNECTABLE, 0x01);
> else if (adapter->current_settings & MGMT_SETTING_CONNECTABLE)
> set_mode(adapter, MGMT_OP_SET_CONNECTABLE, 0x00);
> @@ -9590,19 +9586,19 @@ static void read_commands_complete(uint8_t status, uint16_t length,
> switch (op) {
> case MGMT_OP_ADD_DEVICE:
> DBG("enabling kernel-side connection control");
> - kernel_conn_control = true;
> + kernel_features |= KERNEL_CONN_CONTROL;
> break;
> case MGMT_OP_SET_BLOCKED_KEYS:
> DBG("kernel supports the set_blocked_keys op");
> - kernel_blocked_keys_supported = true;
> + kernel_features |= KERNEL_BLOCKED_KEYS_SUPPORTED;
> break;
> case MGMT_OP_SET_DEF_SYSTEM_CONFIG:
> DBG("kernel supports set system confic");
> - kernel_set_system_config = true;
> + kernel_features |= KERNEL_SET_SYSTEM_CONFIG;
> break;
> case MGMT_OP_READ_EXP_FEATURES_INFO:
> DBG("kernel supports exp features");
> - kernel_exp_features = true;
> + kernel_features |= KERNEL_EXP_FEATURES;
> break;
> default:
> break;
> @@ -9768,3 +9764,8 @@ bool btd_le_connect_before_pairing(void)
>
> return false;
> }
> +
> +bool has_kernel_features(uint32_t features)
> +{
> + return !!(kernel_features & features);
Don't think we need the !! here, the use () should already be enough,
or change it to return (kernel_features & features) ? true : false; if
you think that is more readable.
> +}
> diff --git a/src/adapter.h b/src/adapter.h
> index f8ac20261..b0ed4915f 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -233,3 +233,12 @@ void btd_adapter_for_each_device(struct btd_adapter *adapter,
> void *data);
>
> bool btd_le_connect_before_pairing(void);
> +
> +enum kernel_features {
> + KERNEL_CONN_CONTROL = 1 << 0,
> + KERNEL_BLOCKED_KEYS_SUPPORTED = 1 << 1,
> + KERNEL_SET_SYSTEM_CONFIG = 1 << 2,
> + KERNEL_EXP_FEATURES = 1 << 3,
> +};
> +
> +bool has_kernel_features(uint32_t feature);
> --
> 2.28.0.618.gf4bc123cb7-goog
>
--
Luiz Augusto von Dentz
Hi Luiz,
On Mon, Sep 14, 2020 at 12:59 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Abhishek,
>
> On Fri, Sep 11, 2020 at 3:30 PM Abhishek Pandit-Subedi
> <[email protected]> wrote:
> >
> > Move all the kernel specific feature globals into a single
> > kernel_features bitfield and replace all uses with the bitfield instead.
> > ---
> >
> > Changes in v4: None
> > Changes in v3: None
> > Changes in v2: None
> >
> > src/adapter.c | 59 ++++++++++++++++++++++++++-------------------------
> > src/adapter.h | 9 ++++++++
> > 2 files changed, 39 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/adapter.c b/src/adapter.c
> > index 1435e2bd7..e700a78d5 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -116,13 +116,7 @@ static const struct mgmt_blocked_key_info blocked_keys[] = {
> >
> > static DBusConnection *dbus_conn = NULL;
> >
> > -static bool kernel_conn_control = false;
> > -
> > -static bool kernel_blocked_keys_supported = false;
> > -
> > -static bool kernel_set_system_config = false;
> > -
> > -static bool kernel_exp_features = false;
> > +static uint32_t kernel_features = 0;
> >
> > static GList *adapter_list = NULL;
> > static unsigned int adapter_remaining = 0;
> > @@ -678,7 +672,7 @@ static bool set_discoverable(struct btd_adapter *adapter, uint8_t mode,
> >
> > DBG("sending set mode command for index %u", adapter->dev_id);
> >
> > - if (kernel_conn_control) {
> > + if (has_kernel_features(KERNEL_CONN_CONTROL)) {
> > if (mode)
> > set_mode(adapter, MGMT_OP_SET_CONNECTABLE, mode);
> > else
> > @@ -1334,7 +1328,7 @@ static void trigger_passive_scanning(struct btd_adapter *adapter)
> > * no need to start any discovery. The kernel will keep scanning
> > * as long as devices are in its auto-connection list.
> > */
> > - if (kernel_conn_control)
> > + if (has_kernel_features(KERNEL_CONN_CONTROL))
> > return;
> >
> > /*
> > @@ -1385,7 +1379,7 @@ static void stop_passive_scanning_complete(uint8_t status, uint16_t length,
> > * no need to stop any discovery. The kernel will handle the
> > * auto-connection by itself.
> > */
> > - if (kernel_conn_control)
> > + if (has_kernel_features(KERNEL_CONN_CONTROL))
> > return;
> >
> > /*
> > @@ -2816,7 +2810,7 @@ static void property_set_mode_complete(uint8_t status, uint16_t length,
> >
> > static void clear_discoverable(struct btd_adapter *adapter)
> > {
> > - if (!kernel_conn_control)
> > + if (!has_kernel_features(KERNEL_CONN_CONTROL))
> > return;
> >
> > if (!(adapter->current_settings & MGMT_SETTING_DISCOVERABLE))
> > @@ -2876,7 +2870,7 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
> >
> > break;
> > case MGMT_SETTING_DISCOVERABLE:
> > - if (kernel_conn_control) {
> > + if (has_kernel_features(KERNEL_CONN_CONTROL)) {
> > if (mode) {
> > set_mode(adapter, MGMT_OP_SET_CONNECTABLE,
> > mode);
> > @@ -4193,7 +4187,8 @@ static void load_default_system_params(struct btd_adapter *adapter)
> > size_t len = 0;
> > unsigned int err;
> >
> > - if (!main_opts.default_params.num_entries || !kernel_set_system_config)
> > + if (!main_opts.default_params.num_entries ||
> > + !has_kernel_features(KERNEL_SET_SYSTEM_CONFIG))
> > return;
> >
> > params = malloc0(sizeof(*params) *
> > @@ -4878,7 +4873,7 @@ int adapter_connect_list_add(struct btd_adapter *adapter,
> > * adapter_auto_connect_add() function is used to maintain what to
> > * connect.
> > */
> > - if (kernel_conn_control)
> > + if (has_kernel_features(KERNEL_CONN_CONTROL))
> > return 0;
> >
> > if (g_slist_find(adapter->connect_list, device)) {
> > @@ -4918,7 +4913,7 @@ void adapter_connect_list_remove(struct btd_adapter *adapter,
> > if (device == adapter->connect_le)
> > adapter->connect_le = NULL;
> >
> > - if (kernel_conn_control)
> > + if (has_kernel_features(KERNEL_CONN_CONTROL))
> > return;
> >
> > if (!g_slist_find(adapter->connect_list, device)) {
> > @@ -4980,7 +4975,7 @@ void adapter_whitelist_add(struct btd_adapter *adapter, struct btd_device *dev)
> > {
> > struct mgmt_cp_add_device cp;
> >
> > - if (!kernel_conn_control)
> > + if (!has_kernel_features(KERNEL_CONN_CONTROL))
> > return;
> >
> > memset(&cp, 0, sizeof(cp));
> > @@ -5019,7 +5014,7 @@ void adapter_whitelist_remove(struct btd_adapter *adapter, struct btd_device *de
> > {
> > struct mgmt_cp_remove_device cp;
> >
> > - if (!kernel_conn_control)
> > + if (!has_kernel_features(KERNEL_CONN_CONTROL))
> > return;
> >
> > memset(&cp, 0, sizeof(cp));
> > @@ -5075,7 +5070,7 @@ void adapter_auto_connect_add(struct btd_adapter *adapter,
> > uint8_t bdaddr_type;
> > unsigned int id;
> >
> > - if (!kernel_conn_control)
> > + if (!has_kernel_features(KERNEL_CONN_CONTROL))
> > return;
> >
> > if (g_slist_find(adapter->connect_list, device)) {
> > @@ -5147,7 +5142,7 @@ void adapter_set_device_wakeable(struct btd_adapter *adapter,
> > const bdaddr_t *bdaddr;
> > uint8_t bdaddr_type;
> >
> > - if (!kernel_conn_control)
> > + if (!has_kernel_features(KERNEL_CONN_CONTROL))
> > return;
> >
> > bdaddr = device_get_address(device);
> > @@ -5224,7 +5219,7 @@ void adapter_auto_connect_remove(struct btd_adapter *adapter,
> > uint8_t bdaddr_type;
> > unsigned int id;
> >
> > - if (!kernel_conn_control)
> > + if (!has_kernel_features(KERNEL_CONN_CONTROL))
> > return;
> >
> > if (!g_slist_find(adapter->connect_list, device)) {
> > @@ -6764,7 +6759,7 @@ connect_le:
> > * If kernel background scan is used then the kernel is
> > * responsible for connecting.
> > */
> > - if (kernel_conn_control)
> > + if (has_kernel_features(KERNEL_CONN_CONTROL))
> > return;
> >
> > /*
> > @@ -8964,7 +8959,7 @@ static int clear_devices(struct btd_adapter *adapter)
> > {
> > struct mgmt_cp_remove_device cp;
> >
> > - if (!kernel_conn_control)
> > + if (!has_kernel_features(KERNEL_CONN_CONTROL))
> > return 0;
> >
> > memset(&cp, 0, sizeof(cp));
> > @@ -9282,7 +9277,7 @@ static void read_info_complete(uint8_t status, uint16_t length,
> > (missing_settings & MGMT_SETTING_FAST_CONNECTABLE))
> > set_mode(adapter, MGMT_OP_SET_FAST_CONNECTABLE, 0x01);
> >
> > - if (kernel_exp_features)
> > + if (has_kernel_features(KERNEL_EXP_FEATURES))
> > read_exp_features(adapter);
> >
> > err = adapter_register(adapter);
> > @@ -9403,7 +9398,8 @@ static void read_info_complete(uint8_t status, uint16_t length,
> >
> > set_name(adapter, btd_adapter_get_name(adapter));
> >
> > - if (kernel_blocked_keys_supported && !set_blocked_keys(adapter)) {
> > + if (has_kernel_features(KERNEL_BLOCKED_KEYS_SUPPORTED) &&
> > + !set_blocked_keys(adapter)) {
> > btd_error(adapter->dev_id,
> > "Failed to set blocked keys for index %u",
> > adapter->dev_id);
> > @@ -9414,7 +9410,7 @@ static void read_info_complete(uint8_t status, uint16_t length,
> > !(adapter->current_settings & MGMT_SETTING_BONDABLE))
> > set_mode(adapter, MGMT_OP_SET_BONDABLE, 0x01);
> >
> > - if (!kernel_conn_control)
> > + if (!has_kernel_features(KERNEL_CONN_CONTROL))
> > set_mode(adapter, MGMT_OP_SET_CONNECTABLE, 0x01);
> > else if (adapter->current_settings & MGMT_SETTING_CONNECTABLE)
> > set_mode(adapter, MGMT_OP_SET_CONNECTABLE, 0x00);
> > @@ -9590,19 +9586,19 @@ static void read_commands_complete(uint8_t status, uint16_t length,
> > switch (op) {
> > case MGMT_OP_ADD_DEVICE:
> > DBG("enabling kernel-side connection control");
> > - kernel_conn_control = true;
> > + kernel_features |= KERNEL_CONN_CONTROL;
> > break;
> > case MGMT_OP_SET_BLOCKED_KEYS:
> > DBG("kernel supports the set_blocked_keys op");
> > - kernel_blocked_keys_supported = true;
> > + kernel_features |= KERNEL_BLOCKED_KEYS_SUPPORTED;
> > break;
> > case MGMT_OP_SET_DEF_SYSTEM_CONFIG:
> > DBG("kernel supports set system confic");
> > - kernel_set_system_config = true;
> > + kernel_features |= KERNEL_SET_SYSTEM_CONFIG;
> > break;
> > case MGMT_OP_READ_EXP_FEATURES_INFO:
> > DBG("kernel supports exp features");
> > - kernel_exp_features = true;
> > + kernel_features |= KERNEL_EXP_FEATURES;
> > break;
> > default:
> > break;
> > @@ -9768,3 +9764,8 @@ bool btd_le_connect_before_pairing(void)
> >
> > return false;
> > }
> > +
> > +bool has_kernel_features(uint32_t features)
> > +{
> > + return !!(kernel_features & features);
>
> Don't think we need the !! here, the use () should already be enough,
> or change it to return (kernel_features & features) ? true : false; if
> you think that is more readable.
Will change this to `return (kernel_features & features)`.
>
> > +}
> > diff --git a/src/adapter.h b/src/adapter.h
> > index f8ac20261..b0ed4915f 100644
> > --- a/src/adapter.h
> > +++ b/src/adapter.h
> > @@ -233,3 +233,12 @@ void btd_adapter_for_each_device(struct btd_adapter *adapter,
> > void *data);
> >
> > bool btd_le_connect_before_pairing(void);
> > +
> > +enum kernel_features {
> > + KERNEL_CONN_CONTROL = 1 << 0,
> > + KERNEL_BLOCKED_KEYS_SUPPORTED = 1 << 1,
> > + KERNEL_SET_SYSTEM_CONFIG = 1 << 2,
> > + KERNEL_EXP_FEATURES = 1 << 3,
> > +};
> > +
> > +bool has_kernel_features(uint32_t feature);
> > --
> > 2.28.0.618.gf4bc123cb7-goog
> >
>
>
> --
> Luiz Augusto von Dentz
Any other concerns in this series? If not, will send the new patch
series right away.
Abhishek