diff mbox series

[v4,3/3] Bluetooth: mgmt: add set_quality_report for MGMT_OP_SET_QUALITY_REPORT

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

Checks

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

Commit Message

Joseph Hwang Feb. 15, 2022, 1:35 p.m. UTC
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(-)

Comments

Marcel Holtmann Feb. 17, 2022, 1:04 p.m. UTC | #1
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
Dan Carpenter Feb. 17, 2022, 2:38 p.m. UTC | #2
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
Joseph Hwang March 5, 2022, 7:51 a.m. UTC | #3
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 mbox series

Patch

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)