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: <20210629154652.BlueZ.v5.1.I832f2d744fe2cff0d9749e24c9ec27071fa0b4ed@changeid>
 <20210629154652.BlueZ.v5.3.I5b72c623fb8b002a5e1f000149b362af3c01ab98@changeid>
In-Reply-To: <20210629154652.BlueZ.v5.3.I5b72c623fb8b002a5e1f000149b362af3c01ab98@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 29 Jun 2021 13:25:01 -0700
Message-ID: <CABBYNZJH3Lih+yNdxdre_5zXp4CuNAzek73cTVub0h0rno3YCA@mail.gmail.com>
Subject: Re: [BlueZ PATCH v5 3/3] adapter: set quality report feature
To:     Joseph Hwang <josephsih@chromium.org>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Marcel Holtmann <marcel@holtmann.org>,
        =?UTF-8?Q?Pali_Roh=C3=A1r?= <pali@kernel.org>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>,
        Joseph Hwang <josephsih@google.com>,
        Miao-chen Chou <mcchou@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Joseph,

On Tue, Jun 29, 2021 at 12:47 AM Joseph Hwang <josephsih@chromium.org> wrote:
>
> This patch adds the function to enable/disable the quality report
> experimental feature in the controller through MGMT_OP_SET_EXP_FEATURE.
>
> A user space process can enable/disable the quality report feature
> by sending a property changed signal to the bluetoothd. The bluetoothd
> can set up the signal handlers to handle the signal in a file under
> plugins/ to call this function.
>
> Note that the bluetoothd calls the experimental feature only when
> the quality_report_supported flag is true.
>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> ---
>
> (no changes since v1)
>
>  src/adapter.c | 36 ++++++++++++++++++++++++++++++++++++
>  src/adapter.h |  2 ++
>  2 files changed, 38 insertions(+)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index e2873de46..829d9806b 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -9332,6 +9332,42 @@ static const struct exp_feat {
>         EXP_FEAT(rpa_resolution_uuid, rpa_resolution_func),
>  };
>
> +/* A user space process can enable/disable the quality report feature
> + * by sending a property changed signal to the bluetoothd. The bluetoothd
> + * can set up the signal handlers in a file under plugins/ to call
> + * this function.
> + */
> +void btd_adapter_update_kernel_quality_report(uint8_t action)
> +{
> +       struct mgmt_cp_set_exp_feature cp;
> +       struct btd_adapter *adapter;
> +
> +       adapter = btd_adapter_get_default();
> +       if (!adapter) {
> +               info("No default adapter. Skip enabling quality report.");
> +               return;
> +       }
> +
> +       if (!adapter->quality_report_supported) {
> +               info("quality report feature not supported.");
> +               return;
> +       }
> +
> +       memset(&cp, 0, sizeof(cp));
> +       memcpy(cp.uuid, quality_report_uuid, 16);
> +
> +       cp.action = action;
> +       if (cp.action > 1) {
> +               error("Unexpected quality report action %u", cp.action);
> +               return;
> +       }
> +
> +       mgmt_send(adapter->mgmt, MGMT_OP_SET_EXP_FEATURE, adapter->dev_id,
> +                       sizeof(cp), &cp, NULL, NULL, NULL);
> +       info("update kernel quality report default adapter %d enable %d",
> +               adapter->dev_id, cp.action);
> +}
> +
>  static void read_exp_features_complete(uint8_t status, uint16_t length,
>                                         const void *param, void *user_data)
>  {
> diff --git a/src/adapter.h b/src/adapter.h
> index 60b5e3bcc..001f784e4 100644
> --- a/src/adapter.h
> +++ b/src/adapter.h
> @@ -240,3 +240,5 @@ enum kernel_features {
>  };
>
>  bool btd_has_kernel_features(uint32_t feature);
> +
> +void btd_adapter_update_kernel_quality_report(uint8_t action);

I rather not have these function exposed if there is no upstream code
using it because this may appear on the likes of static analyzer as
something that is never used, also the current policy is that all
experimental features are to be enabled with -E or if experimental is
set on main.conf, also note that experimental features do persist so I
think in the long run we might want to have an interface in the core
to expose the controls of each experimental feature separately, with
that we could replace the current policy of -E to enable all
experimental features to actually enable the interface and then
persist the features that upper layer would like to have enabled in
the storage.

> --
> 2.32.0.93.g670b81a890-goog
>


-- 
Luiz Augusto von Dentz