Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
From: Marcel Holtmann <marcel@holtmann.org>
To: Sathish Narasimman <nsathish41@gmail.com>
Cc: linux-bluetooth@vger.kernel.org,
	Sathish Narasimman <sathish.narasimman@intel.com>
Subject: Re: [PATCH v4 8/8] Bluetooth: Enable controller RPA resolution using Experimental feature
Date: Thu, 16 Jul 2020 09:13:53 +0200
Message-ID: <5CD116F7-EFB3-47A2-B8D5-0012657F10F9@holtmann.org> (raw)
In-Reply-To: <20200713062213.3692-9-sathish.narasimman@intel.com>

Hi Sathish,

> This patch adds support to enable the use of RPA Address resolution
> using expermental feature mgmt command.

everything looks fine, except for this patch. I just prefer to only apply the others if we can apply this one as well.

> Signed-off-by: Sathish Narasimman <sathish.narasimman@intel.com>
> ---
> include/net/bluetooth/hci.h |  1 +
> net/bluetooth/hci_event.c   |  3 ++-
> net/bluetooth/hci_request.c |  6 +++--
> net/bluetooth/mgmt.c        | 52 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 4ff2fc4498f3..cb284365b4c1 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -307,6 +307,7 @@ enum {
> 	HCI_FORCE_BREDR_SMP,
> 	HCI_FORCE_STATIC_ADDR,
> 	HCI_LL_RPA_RESOLUTION,
> +	HCI_ENABLE_RPA_RESOLUTION,

I would call this ENABLE_LL_PRIVAY. It put its more in line with use_ll_privacy and clearly distinct from the LL_RPA_RESOLUTION with is a HCI operational mode.

> 	HCI_CMD_PENDING,
> 	HCI_FORCE_NO_MITM,
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 684c68cb5c76..c8a5e1e4dba2 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -5222,7 +5222,8 @@ static void hci_le_enh_conn_complete_evt(struct hci_dev *hdev,
> 			     le16_to_cpu(ev->latency),
> 			     le16_to_cpu(ev->supervision_timeout));
> 
> -	if (use_ll_privacy(hdev) &&
> +	if (hci_dev_test_flag(hdev, HCI_ENABLE_RPA_RESOLUTION) &&
> +	    use_ll_privacy(hdev) &&
> 	    hci_dev_test_flag(hdev, HCI_LL_RPA_RESOLUTION))

I would leave use_ll_privacy at the top and add the new one after it.

> 		hci_req_disable_address_resolution(hdev);
> }
> diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
> index c3193f7f9ff0..cb44b83539e6 100644
> --- a/net/bluetooth/hci_request.c
> +++ b/net/bluetooth/hci_request.c
> @@ -677,7 +677,8 @@ void hci_req_add_le_scan_disable(struct hci_request *req, bool rpa_le_conn)
> 	}
> 
> 	/* Disable address resolution */
> -	if (use_ll_privacy(hdev) &&
> +	if (hci_dev_test_flag(hdev, HCI_ENABLE_RPA_RESOLUTION) &&
> +	    use_ll_privacy(hdev) &&

Same here.

> 	    hci_dev_test_flag(hdev, HCI_LL_RPA_RESOLUTION) && !rpa_le_conn) {
> 		__u8 enable = 0x00;
> 		hci_req_add(req, HCI_OP_LE_SET_ADDR_RESOLV_ENABLE, 1, &enable);
> @@ -870,7 +871,8 @@ static void hci_req_start_scan(struct hci_request *req, u8 type, u16 interval,
> 		return;
> 	}
> 
> -	if (use_ll_privacy(hdev) && addr_resolv) {
> +	if (hci_dev_test_flag(hdev, HCI_ENABLE_RPA_RESOLUTION) &&
> +	    use_ll_privacy(hdev) && addr_resolv) {

And here.

> 		u8 enable = 0x01;
> 		hci_req_add(req, HCI_OP_LE_SET_ADDR_RESOLV_ENABLE, 1, &enable);
> 	}
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index c292d5de4dc3..fbe02ab5fa05 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -3759,6 +3759,12 @@ static const u8 simult_central_periph_uuid[16] = {
> 	0x96, 0x46, 0xc0, 0x42, 0xb5, 0x10, 0x1b, 0x67,
> };
> 
> +/* 15c0a148-c273-11ea-b3de-0242ac130004 */
> +static const u8 rpa_resolution_uuid[16] = {
> +	0x04, 0x00, 0x13, 0xac, 0x42, 0x02, 0xde, 0xb3,
> +	0xea, 0x11, 0x73, 0xc2, 0x48, 0xa1, 0xc0, 0x15,
> +};
> +
> static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
> 				  void *data, u16 data_len)
> {
> @@ -3795,6 +3801,17 @@ static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
> 		idx++;
> 	}
> 
> +	if (hdev) {

If use_ll_privacy is not available, then we should also not expose this experimental feature.

> +		if (hci_dev_test_flag(hdev, HCI_ENABLE_RPA_RESOLUTION))
> +			flags = BIT(0);
> +		else
> +			flags = 0;
> +

And since we only support the RPA resolution for central mode at the moment, we really now need to disable advertising support. So this one needs to indicate the the supported settings will change when enabled.

> +		memcpy(rp->features[idx].uuid, rpa_resolution_uuid, 16);
> +		rp->features[idx].flags = cpu_to_le32(flags);
> +		idx++;
> +	}
> +
> 	rp->feature_count = cpu_to_le16(idx);
> 
> 	/* After reading the experimental features information, enable
> @@ -3895,6 +3912,41 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> 	}
> #endif
> 
> +	if (!memcmp(cp->uuid, rpa_resolution_uuid, 16)) {
> +		bool val;
> +		int err;
> +
> +		/* Parameters are limited to a single octet */
> +		if (data_len != MGMT_SET_EXP_FEATURE_SIZE + 1)
> +			return mgmt_cmd_status(sk, MGMT_INDEX_NONE,
> +					       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, MGMT_INDEX_NONE,
> +					       MGMT_OP_SET_EXP_FEATURE,
> +					       MGMT_STATUS_INVALID_PARAMS);
> +
> +		val = !!cp->param[0];
> +
> +		if (val)
> +			hci_dev_set_flag(hdev, HCI_ENABLE_RPA_RESOLUTION);
> +		else
> +			hci_dev_clear_flag(hdev, HCI_ENABLE_RPA_RESOLUTION);
> +
> +		memcpy(rp.uuid, rpa_resolution_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, MGMT_INDEX_NONE,
> +					MGMT_OP_SET_EXP_FEATURE, 0,
> +					&rp, sizeof(rp));

The exp_feature_changed event is missing. In addition you need to handle the ZERO_KEY branch which means it will reset all experimental features back to default.

> +
> +		return err;
> +	}
> +
> 	return mgmt_cmd_status(sk, hdev ? hdev->id : MGMT_INDEX_NONE,
> 			       MGMT_OP_SET_EXP_FEATURE,
> 			       MGMT_STATUS_NOT_SUPPORTED);

Regards

Marcel


  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-13  6:22 [PATCH v4 0/8] LL Privacy Support Sathish Narasimman
2020-07-13  6:22 ` [PATCH v4 1/8] Bluetooth: Translate additional address type correctly Sathish Narasimman
2020-07-13  6:22 ` [PATCH v4 2/8] Bluetooth: Configure controller address resolution if available Sathish Narasimman
2020-07-13  6:22 ` [PATCH v4 3/8] Bluetooth: Update resolving list when updating whitelist Sathish Narasimman
2020-07-13  6:22 ` [PATCH v4 4/8] Bluetooth: Translate additional address type during le_conn Sathish Narasimman
2020-07-13  6:22 ` [PATCH v4 5/8] Bluetooth: Let controller creates RPA during le create conn Sathish Narasimman
2020-07-13  6:22 ` [PATCH v4 6/8] Bluetooth: Enable/Disable address resolution " Sathish Narasimman
2020-07-13  6:22 ` [PATCH v4 7/8] Bluetooth: Enable RPA Timeout Sathish Narasimman
2020-07-13  6:22 ` [PATCH v4 8/8] Bluetooth: Enable controller RPA resolution using Experimental feature Sathish Narasimman
2020-07-16  7:13   ` Marcel Holtmann [this message]
2020-07-23 12:23     ` Sathish Narasimman
2020-07-16  5:12 ` [PATCH v4 0/8] LL Privacy Support Sathish Narasimman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5CD116F7-EFB3-47A2-B8D5-0012657F10F9@holtmann.org \
    --to=marcel@holtmann.org \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=nsathish41@gmail.com \
    --cc=sathish.narasimman@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org
	public-inbox-index linux-bluetooth

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git