Hi Luiz,
On Mon, Sep 14, 2020 at 12:59 PM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Abhishek,
>
> On Fri, Sep 11, 2020 at 3:30 PM Abhishek Pandit-Subedi
> <abhishekpandit@xxxxxxxxxxxx> 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
