| Message ID | 20220215133546.2826837-1-josephsih@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v4,1/3] Bluetooth: aosp: surface AOSP quality report through mgmt | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/checkpatch | success | Checkpatch PASS |
| tedd_an/gitlint | success | Gitlint PASS |
| tedd_an/subjectprefix | success | PASS |
Hi Joseph, > This patch adds a new set_quality_report mgmt handler to set > the quality report feature. The feature is removed from the > experimental features at the same time. > > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > --- > > Changes in v4: > - return current settings for set_quality_report. > > Changes in v3: > - This is a new patch to enable the quality report feature. > The reading and setting of the quality report feature are > removed from the experimental features. > > include/net/bluetooth/mgmt.h | 7 ++ > net/bluetooth/mgmt.c | 168 +++++++++++++++-------------------- > 2 files changed, 81 insertions(+), 94 deletions(-) > > diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h > index 83b602636262..74d253ff617a 100644 > --- a/include/net/bluetooth/mgmt.h > +++ b/include/net/bluetooth/mgmt.h > @@ -109,6 +109,7 @@ struct mgmt_rp_read_index_list { > #define MGMT_SETTING_STATIC_ADDRESS 0x00008000 > #define MGMT_SETTING_PHY_CONFIGURATION 0x00010000 > #define MGMT_SETTING_WIDEBAND_SPEECH 0x00020000 > +#define MGMT_SETTING_QUALITY_REPORT 0x00040000 > > #define MGMT_OP_READ_INFO 0x0004 > #define MGMT_READ_INFO_SIZE 0 > @@ -838,6 +839,12 @@ struct mgmt_cp_add_adv_patterns_monitor_rssi { > } __packed; > #define MGMT_ADD_ADV_PATTERNS_MONITOR_RSSI_SIZE 8 > > +#define MGMT_OP_SET_QUALITY_REPORT 0x0057 > +struct mgmt_cp_set_quality_report { > + __u8 action; > +} __packed; > +#define MGMT_SET_QUALITY_REPORT_SIZE 1 > + > #define MGMT_EV_CMD_COMPLETE 0x0001 > struct mgmt_ev_cmd_complete { > __le16 opcode; > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c > index 5e48576041fb..ccd77b94905b 100644 > --- a/net/bluetooth/mgmt.c > +++ b/net/bluetooth/mgmt.c > @@ -857,6 +857,10 @@ static u32 get_supported_settings(struct hci_dev *hdev) > > settings |= MGMT_SETTING_PHY_CONFIGURATION; > > + if (hdev && (aosp_has_quality_report(hdev) || > + hdev->set_quality_report)) > + settings |= MGMT_SETTING_QUALITY_REPORT; > + the hdev check here is useless. The hdev structure is always valid. > return settings; > } > > @@ -928,6 +932,9 @@ static u32 get_current_settings(struct hci_dev *hdev) > if (hci_dev_test_flag(hdev, HCI_WIDEBAND_SPEECH_ENABLED)) > settings |= MGMT_SETTING_WIDEBAND_SPEECH; > > + if (hci_dev_test_flag(hdev, HCI_QUALITY_REPORT)) > + settings |= MGMT_SETTING_QUALITY_REPORT; > + > return settings; > } > > @@ -3871,12 +3878,6 @@ static const u8 debug_uuid[16] = { > }; > #endif > > -/* 330859bc-7506-492d-9370-9a6f0614037f */ > -static const u8 quality_report_uuid[16] = { > - 0x7f, 0x03, 0x14, 0x06, 0x6f, 0x9a, 0x70, 0x93, > - 0x2d, 0x49, 0x06, 0x75, 0xbc, 0x59, 0x08, 0x33, > -}; > - > /* a6695ace-ee7f-4fb9-881a-5fac66c629af */ > static const u8 offload_codecs_uuid[16] = { > 0xaf, 0x29, 0xc6, 0x66, 0xac, 0x5f, 0x1a, 0x88, > @@ -3898,7 +3899,7 @@ static const u8 rpa_resolution_uuid[16] = { > static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev, > void *data, u16 data_len) > { > - char buf[102]; /* Enough space for 5 features: 2 + 20 * 5 */ > + char buf[82]; /* Enough space for 4 features: 2 + 20 * 4 */ > struct mgmt_rp_read_exp_features_info *rp = (void *)buf; > u16 idx = 0; > u32 flags; > @@ -3939,18 +3940,6 @@ static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev, > idx++; > } > > - if (hdev && (aosp_has_quality_report(hdev) || > - hdev->set_quality_report)) { > - if (hci_dev_test_flag(hdev, HCI_QUALITY_REPORT)) > - flags = BIT(0); > - else > - flags = 0; > - > - memcpy(rp->features[idx].uuid, quality_report_uuid, 16); > - rp->features[idx].flags = cpu_to_le32(flags); > - idx++; > - } > - (I see, you copied it from here. Yes, here you need to check for hdev!) > if (hdev && hdev->get_data_path_id) { > if (hci_dev_test_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED)) > flags = BIT(0); > @@ -4163,80 +4152,6 @@ static int set_rpa_resolution_func(struct sock *sk, struct hci_dev *hdev, > return err; > } > > -static int set_quality_report_func(struct sock *sk, struct hci_dev *hdev, > - struct mgmt_cp_set_exp_feature *cp, > - u16 data_len) > -{ > - struct mgmt_rp_set_exp_feature rp; > - bool val, changed; > - int err; > - > - /* Command requires to use a valid controller index */ > - if (!hdev) > - return mgmt_cmd_status(sk, MGMT_INDEX_NONE, > - MGMT_OP_SET_EXP_FEATURE, > - MGMT_STATUS_INVALID_INDEX); > - > - /* Parameters are limited to a single octet */ > - if (data_len != MGMT_SET_EXP_FEATURE_SIZE + 1) > - return mgmt_cmd_status(sk, hdev->id, > - MGMT_OP_SET_EXP_FEATURE, > - MGMT_STATUS_INVALID_PARAMS); > - > - /* Only boolean on/off is supported */ > - if (cp->param[0] != 0x00 && cp->param[0] != 0x01) > - return mgmt_cmd_status(sk, hdev->id, > - MGMT_OP_SET_EXP_FEATURE, > - MGMT_STATUS_INVALID_PARAMS); > - > - hci_req_sync_lock(hdev); > - > - val = !!cp->param[0]; > - changed = (val != hci_dev_test_flag(hdev, HCI_QUALITY_REPORT)); > - > - if (!aosp_has_quality_report(hdev) && !hdev->set_quality_report) { > - err = mgmt_cmd_status(sk, hdev->id, > - MGMT_OP_SET_EXP_FEATURE, > - MGMT_STATUS_NOT_SUPPORTED); > - goto unlock_quality_report; > - } > - > - if (changed) { > - if (hdev->set_quality_report) > - err = hdev->set_quality_report(hdev, val); > - else > - err = aosp_set_quality_report(hdev, val); > - > - if (err) { > - err = mgmt_cmd_status(sk, hdev->id, > - MGMT_OP_SET_EXP_FEATURE, > - MGMT_STATUS_FAILED); > - goto unlock_quality_report; > - } > - > - if (val) > - hci_dev_set_flag(hdev, HCI_QUALITY_REPORT); > - else > - hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT); > - } > - > - bt_dev_dbg(hdev, "quality report enable %d changed %d", val, changed); > - > - memcpy(rp.uuid, quality_report_uuid, 16); > - rp.flags = cpu_to_le32(val ? BIT(0) : 0); > - hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS); > - > - err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_SET_EXP_FEATURE, 0, > - &rp, sizeof(rp)); > - > - if (changed) > - exp_feature_changed(hdev, quality_report_uuid, val, sk); > - > -unlock_quality_report: > - hci_req_sync_unlock(hdev); > - return err; > -} > - > static int set_offload_codec_func(struct sock *sk, struct hci_dev *hdev, > struct mgmt_cp_set_exp_feature *cp, > u16 data_len) > @@ -4363,7 +4278,6 @@ static const struct mgmt_exp_feature { > EXP_FEAT(debug_uuid, set_debug_func), > #endif > EXP_FEAT(rpa_resolution_uuid, set_rpa_resolution_func), > - EXP_FEAT(quality_report_uuid, set_quality_report_func), > EXP_FEAT(offload_codecs_uuid, set_offload_codec_func), > EXP_FEAT(le_simultaneous_roles_uuid, set_le_simultaneous_roles_func), > > @@ -8653,6 +8567,71 @@ static int get_adv_size_info(struct sock *sk, struct hci_dev *hdev, > MGMT_STATUS_SUCCESS, &rp, sizeof(rp)); > } > > +static int set_quality_report(struct sock *sk, struct hci_dev *hdev, > + void *data, u16 data_len) > +{ > + struct mgmt_cp_set_quality_report *cp = data; > + bool enable, changed; > + int err; > + > + /* Command requires to use a valid controller index */ > + if (!hdev) > + return mgmt_cmd_status(sk, MGMT_INDEX_NONE, > + MGMT_OP_SET_QUALITY_REPORT, > + MGMT_STATUS_INVALID_INDEX); > + > + /* Only 0 (off) and 1 (on) is supported */ > + if (cp->action != 0x00 && cp->action != 0x01) > + return mgmt_cmd_status(sk, hdev->id, > + MGMT_OP_SET_QUALITY_REPORT, > + MGMT_STATUS_INVALID_PARAMS); > + > + hci_req_sync_lock(hdev); > + > + enable = !!cp->action; > + changed = (enable != hci_dev_test_flag(hdev, HCI_QUALITY_REPORT)); > + > + if (!aosp_has_quality_report(hdev) && !hdev->set_quality_report) { > + err = mgmt_cmd_status(sk, hdev->id, > + MGMT_OP_SET_QUALITY_REPORT, > + MGMT_STATUS_NOT_SUPPORTED); > + goto unlock_quality_report; > + } > + > + if (changed) { > + if (hdev->set_quality_report) > + err = hdev->set_quality_report(hdev, enable); > + else > + err = aosp_set_quality_report(hdev, enable); > + > + if (err) { > + err = mgmt_cmd_status(sk, hdev->id, > + MGMT_OP_SET_QUALITY_REPORT, > + MGMT_STATUS_FAILED); > + goto unlock_quality_report; > + } > + > + if (enable) > + hci_dev_set_flag(hdev, HCI_QUALITY_REPORT); > + else > + hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT); > + } > + > + bt_dev_dbg(hdev, "quality report enable %d changed %d", > + enable, changed); > + > + err = send_settings_rsp(sk, MGMT_OP_SET_QUALITY_REPORT, hdev); > + if (err < 0) > + goto unlock_quality_report; > + > + if (changed) > + err = new_settings(hdev, sk); > + > +unlock_quality_report: > + hci_req_sync_unlock(hdev); > + return err; > +} > + > static const struct hci_mgmt_handler mgmt_handlers[] = { > { NULL }, /* 0x0000 (no command) */ > { read_version, MGMT_READ_VERSION_SIZE, > @@ -8779,6 +8758,7 @@ static const struct hci_mgmt_handler mgmt_handlers[] = { > { add_adv_patterns_monitor_rssi, > MGMT_ADD_ADV_PATTERNS_MONITOR_RSSI_SIZE, > HCI_MGMT_VAR_LEN }, > + { set_quality_report, MGMT_SET_QUALITY_REPORT_SIZE }, > }; > > void mgmt_index_added(struct hci_dev *hdev) So this patch I actually get merged first. However we still need to figure out if this setting is suppose to survive power off/on cycles. Right now as far as I can tell it is added to hci_dev_clear_volatile_flags and thus needs to be set again after power on. Is this the expected behavior? I don’t think we want that. Since normally all other settings are restored after power on. And it is explicitly mentioned in doc/mgmt-api.txt as well. Regards Marcel
Hi Joseph,
url: https://github.com/0day-ci/linux/commits/Joseph-Hwang/Bluetooth-aosp-surface-AOSP-quality-report-through-mgmt/20220215-213800
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: i386-randconfig-m021-20220214 (https://download.01.org/0day-ci/archive/20220216/202202160942.cEiT1MKh-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
New smatch warnings:
net/bluetooth/mgmt.c:860 get_supported_settings() warn: variable dereferenced before check 'hdev' (see line 826)
vim +/hdev +860 net/bluetooth/mgmt.c
69ab39ea5da03e Johan Hedberg 2011-12-15 816 static u32 get_supported_settings(struct hci_dev *hdev)
69ab39ea5da03e Johan Hedberg 2011-12-15 817 {
69ab39ea5da03e Johan Hedberg 2011-12-15 818 u32 settings = 0;
69ab39ea5da03e Johan Hedberg 2011-12-15 819
69ab39ea5da03e Johan Hedberg 2011-12-15 820 settings |= MGMT_SETTING_POWERED;
b2939475eb6a35 Johan Hedberg 2014-07-30 821 settings |= MGMT_SETTING_BONDABLE;
b1de97d8c06d9d Marcel Holtmann 2014-01-31 822 settings |= MGMT_SETTING_DEBUG_KEYS;
3742abfc4e853f Johan Hedberg 2014-07-08 823 settings |= MGMT_SETTING_CONNECTABLE;
3742abfc4e853f Johan Hedberg 2014-07-08 824 settings |= MGMT_SETTING_DISCOVERABLE;
69ab39ea5da03e Johan Hedberg 2011-12-15 825
ed3fa31f35896b Andre Guedes 2012-07-24 @826 if (lmp_bredr_capable(hdev)) {
1a47aee85f8a08 Johan Hedberg 2013-03-15 827 if (hdev->hci_ver >= BLUETOOTH_VER_1_2)
33c525c0a37abd Johan Hedberg 2012-10-24 828 settings |= MGMT_SETTING_FAST_CONNECTABLE;
69ab39ea5da03e Johan Hedberg 2011-12-15 829 settings |= MGMT_SETTING_BREDR;
69ab39ea5da03e Johan Hedberg 2011-12-15 830 settings |= MGMT_SETTING_LINK_SECURITY;
a82974c9f4ed07 Marcel Holtmann 2013-10-11 831
a82974c9f4ed07 Marcel Holtmann 2013-10-11 832 if (lmp_ssp_capable(hdev)) {
a82974c9f4ed07 Marcel Holtmann 2013-10-11 833 settings |= MGMT_SETTING_SSP;
b560a208cda029 Luiz Augusto von Dentz 2020-08-06 834 if (IS_ENABLED(CONFIG_BT_HS))
d7b7e79688c07b Marcel Holtmann 2012-02-20 835 settings |= MGMT_SETTING_HS;
848566b381e72b Marcel Holtmann 2013-10-01 836 }
e98d2ce293a941 Marcel Holtmann 2014-01-10 837
05b3c3e7905d00 Marcel Holtmann 2014-12-31 838 if (lmp_sc_capable(hdev))
e98d2ce293a941 Marcel Holtmann 2014-01-10 839 settings |= MGMT_SETTING_SECURE_CONN;
4b127bd5f2cc1b Alain Michaud 2020-02-27 840
00bce3fb0642b3 Alain Michaud 2020-03-05 841 if (test_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED,
4b127bd5f2cc1b Alain Michaud 2020-02-27 842 &hdev->quirks))
00bce3fb0642b3 Alain Michaud 2020-03-05 843 settings |= MGMT_SETTING_WIDEBAND_SPEECH;
a82974c9f4ed07 Marcel Holtmann 2013-10-11 844 }
d7b7e79688c07b Marcel Holtmann 2012-02-20 845
eeca6f891305a8 Johan Hedberg 2013-09-25 846 if (lmp_le_capable(hdev)) {
69ab39ea5da03e Johan Hedberg 2011-12-15 847 settings |= MGMT_SETTING_LE;
a3209694f82a22 Johan Hedberg 2014-05-26 848 settings |= MGMT_SETTING_SECURE_CONN;
0f4bd942f13dd1 Johan Hedberg 2014-02-22 849 settings |= MGMT_SETTING_PRIVACY;
93690c227acf08 Marcel Holtmann 2015-03-06 850 settings |= MGMT_SETTING_STATIC_ADDRESS;
cbbdfa6f331980 Sathish Narasimman 2020-07-23 851 settings |= MGMT_SETTING_ADVERTISING;
eeca6f891305a8 Johan Hedberg 2013-09-25 852 }
69ab39ea5da03e Johan Hedberg 2011-12-15 853
eb1904f49d3e11 Marcel Holtmann 2014-07-04 854 if (test_bit(HCI_QUIRK_EXTERNAL_CONFIG, &hdev->quirks) ||
Unchecked dereferences throughout.
eb1904f49d3e11 Marcel Holtmann 2014-07-04 855 hdev->set_bdaddr)
9fc3bfb681bdf5 Marcel Holtmann 2014-07-04 856 settings |= MGMT_SETTING_CONFIGURATION;
9fc3bfb681bdf5 Marcel Holtmann 2014-07-04 857
6244691fec4dd0 Jaganath Kanakkassery 2018-07-19 858 settings |= MGMT_SETTING_PHY_CONFIGURATION;
6244691fec4dd0 Jaganath Kanakkassery 2018-07-19 859
edbb68b1006482 Joseph Hwang 2022-02-15 @860 if (hdev && (aosp_has_quality_report(hdev) ||
^^^^
Checked too late
edbb68b1006482 Joseph Hwang 2022-02-15 861 hdev->set_quality_report))
edbb68b1006482 Joseph Hwang 2022-02-15 862 settings |= MGMT_SETTING_QUALITY_REPORT;
edbb68b1006482 Joseph Hwang 2022-02-15 863
69ab39ea5da03e Johan Hedberg 2011-12-15 864 return settings;
69ab39ea5da03e Johan Hedberg 2011-12-15 865 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Marcel, thank you for reviewing the patches! I have some questions. Please read my replies in the lines below. Thanks! On Thu, Feb 17, 2022 at 9:04 PM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Joseph, > > > This patch adds a new set_quality_report mgmt handler to set > > the quality report feature. The feature is removed from the > > experimental features at the same time. > > > > Signed-off-by: Joseph Hwang <josephsih@chromium.org> > > --- > > > > Changes in v4: > > - return current settings for set_quality_report. > > > > Changes in v3: > > - This is a new patch to enable the quality report feature. > > The reading and setting of the quality report feature are > > removed from the experimental features. > > > > include/net/bluetooth/mgmt.h | 7 ++ > > net/bluetooth/mgmt.c | 168 +++++++++++++++-------------------- > > 2 files changed, 81 insertions(+), 94 deletions(-) > > > > diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h > > index 83b602636262..74d253ff617a 100644 > > --- a/include/net/bluetooth/mgmt.h > > +++ b/include/net/bluetooth/mgmt.h > > @@ -109,6 +109,7 @@ struct mgmt_rp_read_index_list { > > #define MGMT_SETTING_STATIC_ADDRESS 0x00008000 > > #define MGMT_SETTING_PHY_CONFIGURATION 0x00010000 > > #define MGMT_SETTING_WIDEBAND_SPEECH 0x00020000 > > +#define MGMT_SETTING_QUALITY_REPORT 0x00040000 > > > > #define MGMT_OP_READ_INFO 0x0004 > > #define MGMT_READ_INFO_SIZE 0 > > @@ -838,6 +839,12 @@ struct mgmt_cp_add_adv_patterns_monitor_rssi { > > } __packed; > > #define MGMT_ADD_ADV_PATTERNS_MONITOR_RSSI_SIZE 8 > > > > +#define MGMT_OP_SET_QUALITY_REPORT 0x0057 > > +struct mgmt_cp_set_quality_report { > > + __u8 action; > > +} __packed; > > +#define MGMT_SET_QUALITY_REPORT_SIZE 1 > > + > > #define MGMT_EV_CMD_COMPLETE 0x0001 > > struct mgmt_ev_cmd_complete { > > __le16 opcode; > > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c > > index 5e48576041fb..ccd77b94905b 100644 > > --- a/net/bluetooth/mgmt.c > > +++ b/net/bluetooth/mgmt.c > > @@ -857,6 +857,10 @@ static u32 get_supported_settings(struct hci_dev *hdev) > > > > settings |= MGMT_SETTING_PHY_CONFIGURATION; > > > > + if (hdev && (aosp_has_quality_report(hdev) || > > + hdev->set_quality_report)) > > + settings |= MGMT_SETTING_QUALITY_REPORT; > > + > > the hdev check here is useless. The hdev structure is always valid. > > > return settings; > > } > > > > @@ -928,6 +932,9 @@ static u32 get_current_settings(struct hci_dev *hdev) > > if (hci_dev_test_flag(hdev, HCI_WIDEBAND_SPEECH_ENABLED)) > > settings |= MGMT_SETTING_WIDEBAND_SPEECH; > > > > + if (hci_dev_test_flag(hdev, HCI_QUALITY_REPORT)) > > + settings |= MGMT_SETTING_QUALITY_REPORT; > > + > > return settings; > > } > > > > @@ -3871,12 +3878,6 @@ static const u8 debug_uuid[16] = { > > }; > > #endif > > > > -/* 330859bc-7506-492d-9370-9a6f0614037f */ > > -static const u8 quality_report_uuid[16] = { > > - 0x7f, 0x03, 0x14, 0x06, 0x6f, 0x9a, 0x70, 0x93, > > - 0x2d, 0x49, 0x06, 0x75, 0xbc, 0x59, 0x08, 0x33, > > -}; > > - > > /* a6695ace-ee7f-4fb9-881a-5fac66c629af */ > > static const u8 offload_codecs_uuid[16] = { > > 0xaf, 0x29, 0xc6, 0x66, 0xac, 0x5f, 0x1a, 0x88, > > @@ -3898,7 +3899,7 @@ static const u8 rpa_resolution_uuid[16] = { > > static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev, > > void *data, u16 data_len) > > { > > - char buf[102]; /* Enough space for 5 features: 2 + 20 * 5 */ > > + char buf[82]; /* Enough space for 4 features: 2 + 20 * 4 */ > > struct mgmt_rp_read_exp_features_info *rp = (void *)buf; > > u16 idx = 0; > > u32 flags; > > @@ -3939,18 +3940,6 @@ static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev, > > idx++; > > } > > > > - if (hdev && (aosp_has_quality_report(hdev) || > > - hdev->set_quality_report)) { > > - if (hci_dev_test_flag(hdev, HCI_QUALITY_REPORT)) > > - flags = BIT(0); > > - else > > - flags = 0; > > - > > - memcpy(rp->features[idx].uuid, quality_report_uuid, 16); > > - rp->features[idx].flags = cpu_to_le32(flags); > > - idx++; > > - } > > - > > (I see, you copied it from here. Yes, here you need to check for hdev!) > > > if (hdev && hdev->get_data_path_id) { > > if (hci_dev_test_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED)) > > flags = BIT(0); > > @@ -4163,80 +4152,6 @@ static int set_rpa_resolution_func(struct sock *sk, struct hci_dev *hdev, > > return err; > > } > > > > -static int set_quality_report_func(struct sock *sk, struct hci_dev *hdev, > > - struct mgmt_cp_set_exp_feature *cp, > > - u16 data_len) > > -{ > > - struct mgmt_rp_set_exp_feature rp; > > - bool val, changed; > > - int err; > > - > > - /* Command requires to use a valid controller index */ > > - if (!hdev) > > - return mgmt_cmd_status(sk, MGMT_INDEX_NONE, > > - MGMT_OP_SET_EXP_FEATURE, > > - MGMT_STATUS_INVALID_INDEX); > > - > > - /* Parameters are limited to a single octet */ > > - if (data_len != MGMT_SET_EXP_FEATURE_SIZE + 1) > > - return mgmt_cmd_status(sk, hdev->id, > > - MGMT_OP_SET_EXP_FEATURE, > > - MGMT_STATUS_INVALID_PARAMS); > > - > > - /* Only boolean on/off is supported */ > > - if (cp->param[0] != 0x00 && cp->param[0] != 0x01) > > - return mgmt_cmd_status(sk, hdev->id, > > - MGMT_OP_SET_EXP_FEATURE, > > - MGMT_STATUS_INVALID_PARAMS); > > - > > - hci_req_sync_lock(hdev); > > - > > - val = !!cp->param[0]; > > - changed = (val != hci_dev_test_flag(hdev, HCI_QUALITY_REPORT)); > > - > > - if (!aosp_has_quality_report(hdev) && !hdev->set_quality_report) { > > - err = mgmt_cmd_status(sk, hdev->id, > > - MGMT_OP_SET_EXP_FEATURE, > > - MGMT_STATUS_NOT_SUPPORTED); > > - goto unlock_quality_report; > > - } > > - > > - if (changed) { > > - if (hdev->set_quality_report) > > - err = hdev->set_quality_report(hdev, val); > > - else > > - err = aosp_set_quality_report(hdev, val); > > - > > - if (err) { > > - err = mgmt_cmd_status(sk, hdev->id, > > - MGMT_OP_SET_EXP_FEATURE, > > - MGMT_STATUS_FAILED); > > - goto unlock_quality_report; > > - } > > - > > - if (val) > > - hci_dev_set_flag(hdev, HCI_QUALITY_REPORT); > > - else > > - hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT); > > - } > > - > > - bt_dev_dbg(hdev, "quality report enable %d changed %d", val, changed); > > - > > - memcpy(rp.uuid, quality_report_uuid, 16); > > - rp.flags = cpu_to_le32(val ? BIT(0) : 0); > > - hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS); > > - > > - err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_SET_EXP_FEATURE, 0, > > - &rp, sizeof(rp)); > > - > > - if (changed) > > - exp_feature_changed(hdev, quality_report_uuid, val, sk); > > - > > -unlock_quality_report: > > - hci_req_sync_unlock(hdev); > > - return err; > > -} > > - > > static int set_offload_codec_func(struct sock *sk, struct hci_dev *hdev, > > struct mgmt_cp_set_exp_feature *cp, > > u16 data_len) > > @@ -4363,7 +4278,6 @@ static const struct mgmt_exp_feature { > > EXP_FEAT(debug_uuid, set_debug_func), > > #endif > > EXP_FEAT(rpa_resolution_uuid, set_rpa_resolution_func), > > - EXP_FEAT(quality_report_uuid, set_quality_report_func), > > EXP_FEAT(offload_codecs_uuid, set_offload_codec_func), > > EXP_FEAT(le_simultaneous_roles_uuid, set_le_simultaneous_roles_func), > > > > @@ -8653,6 +8567,71 @@ static int get_adv_size_info(struct sock *sk, struct hci_dev *hdev, > > MGMT_STATUS_SUCCESS, &rp, sizeof(rp)); > > } > > > > +static int set_quality_report(struct sock *sk, struct hci_dev *hdev, > > + void *data, u16 data_len) > > +{ > > + struct mgmt_cp_set_quality_report *cp = data; > > + bool enable, changed; > > + int err; > > + > > + /* Command requires to use a valid controller index */ > > + if (!hdev) > > + return mgmt_cmd_status(sk, MGMT_INDEX_NONE, > > + MGMT_OP_SET_QUALITY_REPORT, > > + MGMT_STATUS_INVALID_INDEX); > > + > > + /* Only 0 (off) and 1 (on) is supported */ > > + if (cp->action != 0x00 && cp->action != 0x01) > > + return mgmt_cmd_status(sk, hdev->id, > > + MGMT_OP_SET_QUALITY_REPORT, > > + MGMT_STATUS_INVALID_PARAMS); > > + > > + hci_req_sync_lock(hdev); > > + > > + enable = !!cp->action; > > + changed = (enable != hci_dev_test_flag(hdev, HCI_QUALITY_REPORT)); > > + > > + if (!aosp_has_quality_report(hdev) && !hdev->set_quality_report) { > > + err = mgmt_cmd_status(sk, hdev->id, > > + MGMT_OP_SET_QUALITY_REPORT, > > + MGMT_STATUS_NOT_SUPPORTED); > > + goto unlock_quality_report; > > + } > > + > > + if (changed) { > > + if (hdev->set_quality_report) > > + err = hdev->set_quality_report(hdev, enable); > > + else > > + err = aosp_set_quality_report(hdev, enable); > > + > > + if (err) { > > + err = mgmt_cmd_status(sk, hdev->id, > > + MGMT_OP_SET_QUALITY_REPORT, > > + MGMT_STATUS_FAILED); > > + goto unlock_quality_report; > > + } > > + > > + if (enable) > > + hci_dev_set_flag(hdev, HCI_QUALITY_REPORT); > > + else > > + hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT); > > + } > > + > > + bt_dev_dbg(hdev, "quality report enable %d changed %d", > > + enable, changed); > > + > > + err = send_settings_rsp(sk, MGMT_OP_SET_QUALITY_REPORT, hdev); > > + if (err < 0) > > + goto unlock_quality_report; > > + > > + if (changed) > > + err = new_settings(hdev, sk); > > + > > +unlock_quality_report: > > + hci_req_sync_unlock(hdev); > > + return err; > > +} > > + > > static const struct hci_mgmt_handler mgmt_handlers[] = { > > { NULL }, /* 0x0000 (no command) */ > > { read_version, MGMT_READ_VERSION_SIZE, > > @@ -8779,6 +8758,7 @@ static const struct hci_mgmt_handler mgmt_handlers[] = { > > { add_adv_patterns_monitor_rssi, > > MGMT_ADD_ADV_PATTERNS_MONITOR_RSSI_SIZE, > > HCI_MGMT_VAR_LEN }, > > + { set_quality_report, MGMT_SET_QUALITY_REPORT_SIZE }, > > }; > > > > void mgmt_index_added(struct hci_dev *hdev) > > So this patch I actually get merged first. I do not see this patch getting merged yet. I guess I still need to remove the “hdev” that you mentioned above and re-submit this patch? > > However we still need to figure out if this setting is suppose to survive power off/on cycles. Right now as far as I can tell it is added to hci_dev_clear_volatile_flags and thus needs to be set again after power on. Thank you for pointing this out. Whether the setting should survive power off/on cycles is not mentioned in the AOSP BQR and Intel telemetry specifications. I did a quick test on an Intel platform, the setting does NOT survive over power cycles probably due to the HCI Reset command during power off. Hence, I will need to address this issue by restoring it explicitly. Please let me send separate patches later to address this issue for both Intel and AOSP specs. > > Is this the expected behavior? I don’t think we want that. Since normally all other settings are restored after power on. And it is explicitly mentioned in doc/mgmt-api.txt as well. I will mention this in the Set Powered Command in doc/mgmt-api.txt in the separate patches later. > > Regards > > Marcel > >
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h index 83b602636262..74d253ff617a 100644 --- a/include/net/bluetooth/mgmt.h +++ b/include/net/bluetooth/mgmt.h @@ -109,6 +109,7 @@ struct mgmt_rp_read_index_list { #define MGMT_SETTING_STATIC_ADDRESS 0x00008000 #define MGMT_SETTING_PHY_CONFIGURATION 0x00010000 #define MGMT_SETTING_WIDEBAND_SPEECH 0x00020000 +#define MGMT_SETTING_QUALITY_REPORT 0x00040000 #define MGMT_OP_READ_INFO 0x0004 #define MGMT_READ_INFO_SIZE 0 @@ -838,6 +839,12 @@ struct mgmt_cp_add_adv_patterns_monitor_rssi { } __packed; #define MGMT_ADD_ADV_PATTERNS_MONITOR_RSSI_SIZE 8 +#define MGMT_OP_SET_QUALITY_REPORT 0x0057 +struct mgmt_cp_set_quality_report { + __u8 action; +} __packed; +#define MGMT_SET_QUALITY_REPORT_SIZE 1 + #define MGMT_EV_CMD_COMPLETE 0x0001 struct mgmt_ev_cmd_complete { __le16 opcode; diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c index 5e48576041fb..ccd77b94905b 100644 --- a/net/bluetooth/mgmt.c +++ b/net/bluetooth/mgmt.c @@ -857,6 +857,10 @@ static u32 get_supported_settings(struct hci_dev *hdev) settings |= MGMT_SETTING_PHY_CONFIGURATION; + if (hdev && (aosp_has_quality_report(hdev) || + hdev->set_quality_report)) + settings |= MGMT_SETTING_QUALITY_REPORT; + return settings; } @@ -928,6 +932,9 @@ static u32 get_current_settings(struct hci_dev *hdev) if (hci_dev_test_flag(hdev, HCI_WIDEBAND_SPEECH_ENABLED)) settings |= MGMT_SETTING_WIDEBAND_SPEECH; + if (hci_dev_test_flag(hdev, HCI_QUALITY_REPORT)) + settings |= MGMT_SETTING_QUALITY_REPORT; + return settings; } @@ -3871,12 +3878,6 @@ static const u8 debug_uuid[16] = { }; #endif -/* 330859bc-7506-492d-9370-9a6f0614037f */ -static const u8 quality_report_uuid[16] = { - 0x7f, 0x03, 0x14, 0x06, 0x6f, 0x9a, 0x70, 0x93, - 0x2d, 0x49, 0x06, 0x75, 0xbc, 0x59, 0x08, 0x33, -}; - /* a6695ace-ee7f-4fb9-881a-5fac66c629af */ static const u8 offload_codecs_uuid[16] = { 0xaf, 0x29, 0xc6, 0x66, 0xac, 0x5f, 0x1a, 0x88, @@ -3898,7 +3899,7 @@ static const u8 rpa_resolution_uuid[16] = { static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev, void *data, u16 data_len) { - char buf[102]; /* Enough space for 5 features: 2 + 20 * 5 */ + char buf[82]; /* Enough space for 4 features: 2 + 20 * 4 */ struct mgmt_rp_read_exp_features_info *rp = (void *)buf; u16 idx = 0; u32 flags; @@ -3939,18 +3940,6 @@ static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev, idx++; } - if (hdev && (aosp_has_quality_report(hdev) || - hdev->set_quality_report)) { - if (hci_dev_test_flag(hdev, HCI_QUALITY_REPORT)) - flags = BIT(0); - else - flags = 0; - - memcpy(rp->features[idx].uuid, quality_report_uuid, 16); - rp->features[idx].flags = cpu_to_le32(flags); - idx++; - } - if (hdev && hdev->get_data_path_id) { if (hci_dev_test_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED)) flags = BIT(0); @@ -4163,80 +4152,6 @@ static int set_rpa_resolution_func(struct sock *sk, struct hci_dev *hdev, return err; } -static int set_quality_report_func(struct sock *sk, struct hci_dev *hdev, - struct mgmt_cp_set_exp_feature *cp, - u16 data_len) -{ - struct mgmt_rp_set_exp_feature rp; - bool val, changed; - int err; - - /* Command requires to use a valid controller index */ - if (!hdev) - return mgmt_cmd_status(sk, MGMT_INDEX_NONE, - MGMT_OP_SET_EXP_FEATURE, - MGMT_STATUS_INVALID_INDEX); - - /* Parameters are limited to a single octet */ - if (data_len != MGMT_SET_EXP_FEATURE_SIZE + 1) - return mgmt_cmd_status(sk, hdev->id, - MGMT_OP_SET_EXP_FEATURE, - MGMT_STATUS_INVALID_PARAMS); - - /* Only boolean on/off is supported */ - if (cp->param[0] != 0x00 && cp->param[0] != 0x01) - return mgmt_cmd_status(sk, hdev->id, - MGMT_OP_SET_EXP_FEATURE, - MGMT_STATUS_INVALID_PARAMS); - - hci_req_sync_lock(hdev); - - val = !!cp->param[0]; - changed = (val != hci_dev_test_flag(hdev, HCI_QUALITY_REPORT)); - - if (!aosp_has_quality_report(hdev) && !hdev->set_quality_report) { - err = mgmt_cmd_status(sk, hdev->id, - MGMT_OP_SET_EXP_FEATURE, - MGMT_STATUS_NOT_SUPPORTED); - goto unlock_quality_report; - } - - if (changed) { - if (hdev->set_quality_report) - err = hdev->set_quality_report(hdev, val); - else - err = aosp_set_quality_report(hdev, val); - - if (err) { - err = mgmt_cmd_status(sk, hdev->id, - MGMT_OP_SET_EXP_FEATURE, - MGMT_STATUS_FAILED); - goto unlock_quality_report; - } - - if (val) - hci_dev_set_flag(hdev, HCI_QUALITY_REPORT); - else - hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT); - } - - bt_dev_dbg(hdev, "quality report enable %d changed %d", val, changed); - - memcpy(rp.uuid, quality_report_uuid, 16); - rp.flags = cpu_to_le32(val ? BIT(0) : 0); - hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS); - - err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_SET_EXP_FEATURE, 0, - &rp, sizeof(rp)); - - if (changed) - exp_feature_changed(hdev, quality_report_uuid, val, sk); - -unlock_quality_report: - hci_req_sync_unlock(hdev); - return err; -} - static int set_offload_codec_func(struct sock *sk, struct hci_dev *hdev, struct mgmt_cp_set_exp_feature *cp, u16 data_len) @@ -4363,7 +4278,6 @@ static const struct mgmt_exp_feature { EXP_FEAT(debug_uuid, set_debug_func), #endif EXP_FEAT(rpa_resolution_uuid, set_rpa_resolution_func), - EXP_FEAT(quality_report_uuid, set_quality_report_func), EXP_FEAT(offload_codecs_uuid, set_offload_codec_func), EXP_FEAT(le_simultaneous_roles_uuid, set_le_simultaneous_roles_func), @@ -8653,6 +8567,71 @@ static int get_adv_size_info(struct sock *sk, struct hci_dev *hdev, MGMT_STATUS_SUCCESS, &rp, sizeof(rp)); } +static int set_quality_report(struct sock *sk, struct hci_dev *hdev, + void *data, u16 data_len) +{ + struct mgmt_cp_set_quality_report *cp = data; + bool enable, changed; + int err; + + /* Command requires to use a valid controller index */ + if (!hdev) + return mgmt_cmd_status(sk, MGMT_INDEX_NONE, + MGMT_OP_SET_QUALITY_REPORT, + MGMT_STATUS_INVALID_INDEX); + + /* Only 0 (off) and 1 (on) is supported */ + if (cp->action != 0x00 && cp->action != 0x01) + return mgmt_cmd_status(sk, hdev->id, + MGMT_OP_SET_QUALITY_REPORT, + MGMT_STATUS_INVALID_PARAMS); + + hci_req_sync_lock(hdev); + + enable = !!cp->action; + changed = (enable != hci_dev_test_flag(hdev, HCI_QUALITY_REPORT)); + + if (!aosp_has_quality_report(hdev) && !hdev->set_quality_report) { + err = mgmt_cmd_status(sk, hdev->id, + MGMT_OP_SET_QUALITY_REPORT, + MGMT_STATUS_NOT_SUPPORTED); + goto unlock_quality_report; + } + + if (changed) { + if (hdev->set_quality_report) + err = hdev->set_quality_report(hdev, enable); + else + err = aosp_set_quality_report(hdev, enable); + + if (err) { + err = mgmt_cmd_status(sk, hdev->id, + MGMT_OP_SET_QUALITY_REPORT, + MGMT_STATUS_FAILED); + goto unlock_quality_report; + } + + if (enable) + hci_dev_set_flag(hdev, HCI_QUALITY_REPORT); + else + hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT); + } + + bt_dev_dbg(hdev, "quality report enable %d changed %d", + enable, changed); + + err = send_settings_rsp(sk, MGMT_OP_SET_QUALITY_REPORT, hdev); + if (err < 0) + goto unlock_quality_report; + + if (changed) + err = new_settings(hdev, sk); + +unlock_quality_report: + hci_req_sync_unlock(hdev); + return err; +} + static const struct hci_mgmt_handler mgmt_handlers[] = { { NULL }, /* 0x0000 (no command) */ { read_version, MGMT_READ_VERSION_SIZE, @@ -8779,6 +8758,7 @@ static const struct hci_mgmt_handler mgmt_handlers[] = { { add_adv_patterns_monitor_rssi, MGMT_ADD_ADV_PATTERNS_MONITOR_RSSI_SIZE, HCI_MGMT_VAR_LEN }, + { set_quality_report, MGMT_SET_QUALITY_REPORT_SIZE }, }; void mgmt_index_added(struct hci_dev *hdev)
This patch adds a new set_quality_report mgmt handler to set the quality report feature. The feature is removed from the experimental features at the same time. Signed-off-by: Joseph Hwang <josephsih@chromium.org> --- Changes in v4: - return current settings for set_quality_report. Changes in v3: - This is a new patch to enable the quality report feature. The reading and setting of the quality report feature are removed from the experimental features. include/net/bluetooth/mgmt.h | 7 ++ net/bluetooth/mgmt.c | 168 +++++++++++++++-------------------- 2 files changed, 81 insertions(+), 94 deletions(-)