Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200911223037.4127188-1-abhishekpandit@chromium.org> <20200911153010.Bluez.v4.1.Ib9712d2bf5d4b3f90c5bc835742aea8c7cd239e4@changeid>
In-Reply-To: <20200911153010.Bluez.v4.1.Ib9712d2bf5d4b3f90c5bc835742aea8c7cd239e4@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 14 Sep 2020 12:59:47 -0700
Message-ID: <CABBYNZKk=tgUgF6UrYfGxS5fv9b9WBbFAo-iZiMe9z1OECJUEQ@mail.gmail.com>
Subject: Re: [Bluez PATCH v4 1/4] adapter: Refactor kernel feature globals
To:     Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Abhishek,

On Fri, Sep 11, 2020 at 3:30 PM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> 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