| Message ID | 20210621182149.BlueZ.v2.3.I5b72c623fb8b002a5e1f000149b362af3c01ab98@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [BlueZ,v2,1/3] monitor: add new Intel extended telemetry events | expand |
Hi Joseph, On Mon, Jun 21, 2021 at 3:23 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); It doesn't seem this is being used anywhere, in the patch description it mentions a plugin is handling this via a signal but we could actually make the core do that directly, in fact having a plugin handling posix signals might be a bad idea. If this is something we don't need to change at runtime I would expect it to be configurable over main.conf, or are there legitimate reasons to not have the controller generating these events all the time? > -- > 2.32.0.288.g62a8d224e6-goog >
Please read the replies in the lines below. Thanks! On Tue, Jun 22, 2021 at 2:25 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Joseph, > > On Mon, Jun 21, 2021 at 3:23 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); > > It doesn't seem this is being used anywhere, in the patch description It is not used elsewhere except in plugins/chromium.c. The function is placed here so that other user processes in the Linux community can use it very easily. Please refer to how it is invoked in chrome os https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/bluez/current/plugins/chromium.c;l=748?q=btd_adapter_update_kernel_quality_report&ss=chromiumos This quality report experimental feature (CONFIG_BT_FEATURE_QUALITY_REPORT) follows what was implemented for the debug log experimental feature (CONFIG_BT_FEATURE_DEBUG). > it mentions a plugin is handling this via a signal but we could > actually make the core do that directly, in fact having a plugin > handling posix signals might be a bad idea. If this is something we > don't need to change at runtime I would expect it to be configurable In chrome os, this quality report experimental feature, just like the other debug log experimental feature, is required to change at run time. Both experimental features are enabled/disabled at run time with the same mechanism -- a user space process sends the dbus signals to enable/disable the features. > over main.conf, or are there legitimate reasons to not have the > controller generating these events all the time? Another reason not to generate the events all the time is to avoid the overhead on the BT controller and the stack if the feature is not required. > > > -- > > 2.32.0.288.g62a8d224e6-goog > > > > > -- > Luiz Augusto von Dentz
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);
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(+)