[PATCH] Add support for LE ping feature

[PATCH] Add support for LE ping feature

[SpoorthiX K]

From: SpoorthiX <spoorthix.k@intel.com>

Changes made to add HCI Write Authenticated Payload timeout
command for LE Ping feature.
As per the Core Specification 5.0 Volume 2 Part E Section 7.3.94,
the following code changes implements
HCI Write Authenticated Payload timeout command for LE Ping feature.

Signed-off-by: SpoorthiX <spoorthix.k@intel.com>
---
 include/net/bluetooth/hci.h |  7 +++++++
 net/bluetooth/hci_event.c   | 21 +++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c36dc1e..ec37c19 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1130,6 +1130,13 @@ struct hci_cp_write_sc_support {
 	__u8	support;
 } __packed;
 
+#define HCI_OP_WRITE_AUTH_PAYLOAD_TO    0x0c7c
+struct hci_cp_write_auth_payload_to {
+        __u16     conn_handle;
+        __u16      timeout;
+} __packed;
+
+
 #define HCI_OP_READ_LOCAL_OOB_EXT_DATA	0x0c7d
 struct hci_rp_read_local_oob_ext_data {
 	__u8     status;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 609fd68..ae14927 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -38,6 +38,7 @@
 
 #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
 		 "\x00\x00\x00\x00\x00\x00\x00\x00"
+#define AUTHENTICATED_PAYLOAD_TIMEOUT 0x0BB8
 
 /* Handle HCI Event packets */
 
@@ -4815,6 +4816,22 @@ static void hci_disconn_phylink_complete_evt(struct hci_dev *hdev,
 }
 #endif
 
+static void hci_write_auth_payload_to (struct hci_dev *hdev, struct hci_conn *conn)
+{
+        struct hci_cp_write_auth_payload_to cp;
+
+        /* Check for existing LE link established between local
+         * and remote device, also check the controller capability
+         * for LE Ping.
+         */
+        if ((conn->type == LE_LINK) && (lmp_ping_capable(hdev)))  {
+                cp.conn_handle = cpu_to_le16(conn->handle);
+                cp.timeout = AUTHENTICATED_PAYLOAD_TIMEOUT;
+                hci_send_cmd(conn->hdev, HCI_OP_WRITE_AUTH_PAYLOAD_TO,
+			sizeof(cp), &cp);
+        }
+}
+
 static void le_conn_complete_evt(struct hci_dev *hdev, u8 status,
 			bdaddr_t *bdaddr, u8 bdaddr_type, u8 role, u16 handle,
 			u16 interval, u16 latency, u16 supervision_timeout)
@@ -4972,6 +4989,10 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status,
 		}
 	}
 
+         /* Set the default Authenticated Payload Timeout after
+          * an LE Link is established.
+          */
+	hci_write_auth_payload_to (hdev, conn);
 unlock:
 	hci_update_background_scan(hdev);
 	hci_dev_unlock(hdev);
-- 
1.9.1

Re: [PATCH] Add support for LE ping feature

[Marcel Holtmann]

Hi,

> Changes made to add HCI Write Authenticated Payload timeout
> command for LE Ping feature.
> As per the Core Specification 5.0 Volume 2 Part E Section 7.3.94,
> the following code changes implements
> HCI Write Authenticated Payload timeout command for LE Ping feature.
> 
> Signed-off-by: SpoorthiX <spoorthix.k@intel.com>

we prefer proper names and not some nicknames or abbreviations. So please configure your .gitconfig accordingly.

> ---
> include/net/bluetooth/hci.h |  7 +++++++
> net/bluetooth/hci_event.c   | 21 +++++++++++++++++++++
> 2 files changed, 28 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index c36dc1e..ec37c19 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1130,6 +1130,13 @@ struct hci_cp_write_sc_support {
> 	__u8	support;
> } __packed;
> 
> +#define HCI_OP_WRITE_AUTH_PAYLOAD_TO    0x0c7c
> +struct hci_cp_write_auth_payload_to {
> +        __u16     conn_handle;
> +        __u16      timeout;
> +} __packed;
> +
> +

Please follow the coding style and not just randomly code things.

In addition please use proper endian notation.

> #define HCI_OP_READ_LOCAL_OOB_EXT_DATA	0x0c7d
> struct hci_rp_read_local_oob_ext_data {
> 	__u8     status;
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 609fd68..ae14927 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -38,6 +38,7 @@
> 
> #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
> 		 "\x00\x00\x00\x00\x00\x00\x00\x00"
> +#define AUTHENTICATED_PAYLOAD_TIMEOUT 0x0BB8

I am not even sure this is the right location for this constant. Since frankly there needs to be some default value configured in hci_dev and it might also need at least some debugfs setting to change it.

> 
> /* Handle HCI Event packets */
> 
> @@ -4815,6 +4816,22 @@ static void hci_disconn_phylink_complete_evt(struct hci_dev *hdev,
> }
> #endif
> 
> +static void hci_write_auth_payload_to (struct hci_dev *hdev, struct hci_conn *conn)
> +{
> +        struct hci_cp_write_auth_payload_to cp;
> +
> +        /* Check for existing LE link established between local
> +         * and remote device, also check the controller capability
> +         * for LE Ping.
> +         */
> +        if ((conn->type == LE_LINK) && (lmp_ping_capable(hdev)))  {

No extra () needed here.

> +                cp.conn_handle = cpu_to_le16(conn->handle);
> +                cp.timeout = AUTHENTICATED_PAYLOAD_TIMEOUT;
> +                hci_send_cmd(conn->hdev, HCI_OP_WRITE_AUTH_PAYLOAD_TO,
> +			sizeof(cp), &cp);
> +        }
> +}
> +
> static void le_conn_complete_evt(struct hci_dev *hdev, u8 status,
> 			bdaddr_t *bdaddr, u8 bdaddr_type, u8 role, u16 handle,
> 			u16 interval, u16 latency, u16 supervision_timeout)
> @@ -4972,6 +4989,10 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status,
> 		}
> 	}
> 
> +         /* Set the default Authenticated Payload Timeout after
> +          * an LE Link is established.
> +          */
> +	hci_write_auth_payload_to (hdev, conn);

I am not sure just sending this value acceptable. We need to store a per hci_conn default and change it once this succeeds. So a lot more code is needed to make this clean and useful.

Also when an LE connection is complete, we need to ensure the right commands are send in the right order and status update to higher layers are only given once they complete.

> unlock:
> 	hci_update_background_scan(hdev);
> 	hci_dev_unlock(hdev);
> -- 
> 1.9.1
> 

Regards

Marcel

Re: [PATCH] Add support for LE ping feature

[kbuild test robot]

Hi SpoorthiX,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on v5.1-rc1 next-20190318]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/SpoorthiX-K/Add-support-for-LE-ping-feature/20190319-031345
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'


sparse warnings: (new ones prefixed by >>)

   include/linux/overflow.h:285:13: sparse: undefined identifier '__builtin_mul_overflow'
   include/linux/overflow.h:285:13: sparse: incorrect type in conditional
   include/linux/overflow.h:285:13:    got void
   include/linux/overflow.h:287:13: sparse: undefined identifier '__builtin_add_overflow'
   include/linux/overflow.h:287:13: sparse: incorrect type in conditional
   include/linux/overflow.h:287:13:    got void
   include/linux/overflow.h:285:13: sparse: not a function <noident>
   include/linux/overflow.h:285:13: sparse: incorrect type in conditional
   include/linux/overflow.h:285:13:    got void
   include/linux/overflow.h:287:13: sparse: not a function <noident>
   include/linux/overflow.h:287:13: sparse: incorrect type in conditional
   include/linux/overflow.h:287:13:    got void
>> net/bluetooth/hci_event.c:4828:32: sparse: incorrect type in assignment (different base types) @@    expected unsigned short [unsigned] [usertype] conn_handle @@    got  short [unsigned] [usertype] conn_handle @@
   net/bluetooth/hci_event.c:4828:32:    expected unsigned short [unsigned] [usertype] conn_handle
   net/bluetooth/hci_event.c:4828:32:    got restricted __le16 [usertype] <noident>
   include/linux/overflow.h:285:13: sparse: call with no type!
   include/linux/overflow.h:287:13: sparse: call with no type!

vim +4828 net/bluetooth/hci_event.c

  4818	
  4819	static void hci_write_auth_payload_to (struct hci_dev *hdev, struct hci_conn *conn)
  4820	{
  4821	        struct hci_cp_write_auth_payload_to cp;
  4822	
  4823	        /* Check for existing LE link established between local
  4824	         * and remote device, also check the controller capability
  4825	         * for LE Ping.
  4826	         */
  4827	        if ((conn->type == LE_LINK) && (lmp_ping_capable(hdev)))  {
> 4828	                cp.conn_handle = cpu_to_le16(conn->handle);
  4829	                cp.timeout = AUTHENTICATED_PAYLOAD_TIMEOUT;
  4830	                hci_send_cmd(conn->hdev, HCI_OP_WRITE_AUTH_PAYLOAD_TO,
  4831				sizeof(cp), &cp);
  4832	        }
  4833	}
  4834	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH] Add Support for LE Ping feature

[SpoorthiX K]

From: Spoorthi Ravishankar Koppad <spoorthix.k@intel.com>

Changes made to add HCI Write Authenticated Payload timeout
command for LE Ping feature.
As per the Core Specification 5.0 Volume 2 Part E Section 7.3.94,
the following code changes implements
HCI Write Authenticated Payload timeout command for LE Ping feature.

Signed-off-by: Spoorthi Ravishankar Koppad <spoorthix.k@intel.com>
---
 include/net/bluetooth/hci.h      | 10 +++++++++
 include/net/bluetooth/hci_core.h |  1 +
 net/bluetooth/hci_core.c         |  1 +
 net/bluetooth/hci_debugfs.c      | 31 +++++++++++++++++++++++++++
 net/bluetooth/hci_event.c        | 46 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 89 insertions(+)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index fbba43e..a9e7470d9 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1142,6 +1142,16 @@ struct hci_cp_write_sc_support {
 	__u8	support;
 } __packed;
 
+#define HCI_OP_WRITE_AUTH_PAYLOAD_TO    0x0c7c
+struct hci_cp_write_auth_payload_to {
+	__le16   conn_handle;
+	__u16    timeout;
+} __packed;
+struct hci_rp_write_auth_payload_to {
+	__u8     status;
+	__le16   conn_handle;
+} __packed;
+
 #define HCI_OP_READ_LOCAL_OOB_EXT_DATA	0x0c7d
 struct hci_rp_read_local_oob_ext_data {
 	__u8     status;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 094e61e..f957bb0 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -421,6 +421,7 @@ struct hci_dev {
 	__u32			rpa_timeout;
 	struct delayed_work	rpa_expired;
 	bdaddr_t		rpa;
+	__u16			le_auth_payload_timeout;
 
 #if IS_ENABLED(CONFIG_BT_LEDS)
 	struct led_trigger	*power_led;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d6b2540..a00358b 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3193,6 +3193,7 @@ struct hci_dev *hci_alloc_dev(void)
 	hdev->le_max_tx_time = 0x0148;
 	hdev->le_max_rx_len = 0x001b;
 	hdev->le_max_rx_time = 0x0148;
+	hdev->le_auth_payload_timeout = 0xbb8;
 	hdev->le_max_key_size = SMP_MAX_ENC_KEY_SIZE;
 	hdev->le_min_key_size = SMP_MIN_ENC_KEY_SIZE;
 	hdev->le_tx_def_phys = HCI_LE_SET_PHY_1M;
diff --git a/net/bluetooth/hci_debugfs.c b/net/bluetooth/hci_debugfs.c
index 51f5b1e..2776708 100644
--- a/net/bluetooth/hci_debugfs.c
+++ b/net/bluetooth/hci_debugfs.c
@@ -941,6 +941,35 @@ static int adv_max_interval_get(void *data, u64 *val)
 DEFINE_SIMPLE_ATTRIBUTE(adv_max_interval_fops, adv_max_interval_get,
 			adv_max_interval_set, "%llu\n");
 
+static int authenticated_payload_timeout_set(void *data, u64 val)
+{
+	struct hci_dev *hdev = data;
+
+	if (val < 0x0001 || val > 0xffff)
+		return -EINVAL;
+
+	hci_dev_lock(hdev);
+	hdev->le_auth_payload_timeout = val;
+	hci_dev_unlock(hdev);
+
+	return 0;
+}
+
+static int authenticated_payload_timeout_get(void *data, u64 *val)
+{
+	struct hci_dev *hdev = data;
+
+	hci_dev_lock(hdev);
+	*val = hdev->le_auth_payload_timeout;
+	hci_dev_unlock(hdev);
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(authenticated_payload_timeout_fops,
+			authenticated_payload_timeout_get,
+			authenticated_payload_timeout_set, "%llu\n");
+
 DEFINE_QUIRK_ATTRIBUTE(quirk_strict_duplicate_filter,
 		       HCI_QUIRK_STRICT_DUPLICATE_FILTER);
 DEFINE_QUIRK_ATTRIBUTE(quirk_simultaneous_discovery,
@@ -992,6 +1021,8 @@ void hci_debugfs_create_le(struct hci_dev *hdev)
 			    &adv_min_interval_fops);
 	debugfs_create_file("adv_max_interval", 0644, hdev->debugfs, hdev,
 			    &adv_max_interval_fops);
+	debugfs_create_file("authenticated_payload_timeout",
+			    0644, hdev->debugfs, hdev,
+			    &authenticated_payload_timeout_fops);
 	debugfs_create_u16("discov_interleaved_timeout", 0644, hdev->debugfs,
 			   &hdev->discov_interleaved_timeout);
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 609fd68..001ece2 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -564,6 +564,31 @@ static void hci_cc_read_local_version(struct hci_dev *hdev, struct sk_buff *skb)
 	}
 }
 
+static void hci_cc_write_auth_payload_timeout(struct hci_dev *hdev,
+					      struct sk_buff *skb)
+{
+	struct hci_rp_write_auth_payload_to *rp = (void *)skb->data;
+	struct hci_cp_write_auth_payload_to *sent;
+
+	BT_DBG("%s status 0x%2.2x", hdev->name, rp->status);
+
+	if (rp->status)
+		return;
+
+	sent = hci_sent_cmd_data(hdev, HCI_OP_WRITE_AUTH_PAYLOAD_TO);
+	if (!sent)
+		return;
+
+	hci_dev_lock(hdev);
+
+	if (!rp->status)
+		hdev->features[1][0] |= LMP_PING;
+	else
+		hdev->features[1][0] &= ~LMP_PING;
+
+	hci_dev_unlock(hdev);
+}
+
 static void hci_cc_read_local_commands(struct hci_dev *hdev,
 				       struct sk_buff *skb)
 {
@@ -3170,6 +3195,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb,
 		hci_cc_write_sc_support(hdev, skb);
 		break;
 
+	case HCI_OP_WRITE_AUTH_PAYLOAD_TO:
+		hci_cc_write_auth_payload_timeout(hdev, skb);
+		break;
+
 	case HCI_OP_READ_LOCAL_VERSION:
 		hci_cc_read_local_version(hdev, skb);
 		break;
@@ -4971,7 +5000,24 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status,
 			params->conn = NULL;
 		}
 	}
+	/* Set the default Authenticated Payload Timeout after
+	 * an LE Link is established. As per Core Spec v5.0, Vol 2, Part B
+	 * Section 3.3, the HCI command WRITE_AUTH_PAYLOAD_TIMEOUT should be
+	 * sent when the link is active and Encryption is enabled, the conn
+	 * type must be LE and controller must support LE Ping.
+	 */
+	if (conn->type == LE_LINK && lmp_ping_capable(hdev) &&
+	    test_bit(HCI_ENCRYPT, &hdev->flags)) {
+		struct hci_cp_write_auth_payload_to cp;
 
+		cp.conn_handle = cpu_to_le16(conn->handle);
+		cp.timeout = cpu_to_le16(hdev->le_auth_payload_timeout);
+		hci_send_cmd(conn->hdev, HCI_OP_WRITE_AUTH_PAYLOAD_TO,
+			     sizeof(cp), &cp);
+	} else {
+		conn->state = BT_CONNECTED;
+		hci_connect_cfm(conn, status);
+	}
 unlock:
 	hci_update_background_scan(hdev);
 	hci_dev_unlock(hdev);
-- 
1.9.1

Re: [PATCH] Add Support for LE Ping feature

[Marcel Holtmann]

Hi Spoorthi,

> Changes made to add HCI Write Authenticated Payload timeout
> command for LE Ping feature.
> As per the Core Specification 5.0 Volume 2 Part E Section 7.3.94,
> the following code changes implements
> HCI Write Authenticated Payload timeout command for LE Ping feature.
> 
> Signed-off-by: Spoorthi Ravishankar Koppad <spoorthix.k@intel.com>
> ---
> include/net/bluetooth/hci.h      | 10 +++++++++
> include/net/bluetooth/hci_core.h |  1 +
> net/bluetooth/hci_core.c         |  1 +
> net/bluetooth/hci_debugfs.c      | 31 +++++++++++++++++++++++++++
> net/bluetooth/hci_event.c        | 46 ++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 89 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index fbba43e..a9e7470d9 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1142,6 +1142,16 @@ struct hci_cp_write_sc_support {
> 	__u8	support;
> } __packed;
> 
> +#define HCI_OP_WRITE_AUTH_PAYLOAD_TO    0x0c7c
> +struct hci_cp_write_auth_payload_to {
> +	__le16   conn_handle;
> +	__u16    timeout;
> +} __packed;
> +struct hci_rp_write_auth_payload_to {
> +	__u8     status;
> +	__le16   conn_handle;
> +} __packed;
> +

we never use conn_handle. It has always been handle. And HCI is a little-endian API. So you can not have __u16 in it.

> #define HCI_OP_READ_LOCAL_OOB_EXT_DATA	0x0c7d
> struct hci_rp_read_local_oob_ext_data {
> 	__u8     status;
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 094e61e..f957bb0 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -421,6 +421,7 @@ struct hci_dev {
> 	__u32			rpa_timeout;
> 	struct delayed_work	rpa_expired;
> 	bdaddr_t		rpa;
> +	__u16			le_auth_payload_timeout;

It is not just LE specific. The BR/EDR Secure Connections also have a payload timeout.

> #if IS_ENABLED(CONFIG_BT_LEDS)
> 	struct led_trigger	*power_led;
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index d6b2540..a00358b 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3193,6 +3193,7 @@ struct hci_dev *hci_alloc_dev(void)
> 	hdev->le_max_tx_time = 0x0148;
> 	hdev->le_max_rx_len = 0x001b;
> 	hdev->le_max_rx_time = 0x0148;
> +	hdev->le_auth_payload_timeout = 0xbb8;
> 	hdev->le_max_key_size = SMP_MAX_ENC_KEY_SIZE;
> 	hdev->le_min_key_size = SMP_MIN_ENC_KEY_SIZE;
> 	hdev->le_tx_def_phys = HCI_LE_SET_PHY_1M;
> diff --git a/net/bluetooth/hci_debugfs.c b/net/bluetooth/hci_debugfs.c
> index 51f5b1e..2776708 100644
> --- a/net/bluetooth/hci_debugfs.c
> +++ b/net/bluetooth/hci_debugfs.c
> @@ -941,6 +941,35 @@ static int adv_max_interval_get(void *data, u64 *val)
> DEFINE_SIMPLE_ATTRIBUTE(adv_max_interval_fops, adv_max_interval_get,
> 			adv_max_interval_set, "%llu\n");
> 
> +static int authenticated_payload_timeout_set(void *data, u64 val)
> +{

Use auth_payload_timeout_set as name.

> +	struct hci_dev *hdev = data;
> +
> +	if (val < 0x0001 || val > 0xffff)
> +		return -EINVAL;
> +
> +	hci_dev_lock(hdev);
> +	hdev->le_auth_payload_timeout = val;
> +	hci_dev_unlock(hdev);
> +
> +	return 0;
> +}
> +
> +static int authenticated_payload_timeout_get(void *data, u64 *val)
> +{
> +	struct hci_dev *hdev = data;
> +
> +	hci_dev_lock(hdev);
> +	*val = hdev->le_auth_payload_timeout;
> +	hci_dev_unlock(hdev);
> +
> +	return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(authenticated_payload_timeout_fops,
> +			authenticated_payload_timeout_get,
> +			authenticated_payload_timeout_set, "%llu\n");
> +
> DEFINE_QUIRK_ATTRIBUTE(quirk_strict_duplicate_filter,
> 		       HCI_QUIRK_STRICT_DUPLICATE_FILTER);
> DEFINE_QUIRK_ATTRIBUTE(quirk_simultaneous_discovery,
> @@ -992,6 +1021,8 @@ void hci_debugfs_create_le(struct hci_dev *hdev)
> 			    &adv_min_interval_fops);
> 	debugfs_create_file("adv_max_interval", 0644, hdev->debugfs, hdev,
> 			    &adv_max_interval_fops);
> +	debugfs_create_file("authenticated_payload_timeout",
> +			    0644, hdev->debugfs, hdev,
> +			    &authenticated_payload_timeout_fops);
> 	debugfs_create_u16("discov_interleaved_timeout", 0644, hdev->debugfs,
> 			   &hdev->discov_interleaved_timeout);
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 609fd68..001ece2 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -564,6 +564,31 @@ static void hci_cc_read_local_version(struct hci_dev *hdev, struct sk_buff *skb)
> 	}
> }
> 
> +static void hci_cc_write_auth_payload_timeout(struct hci_dev *hdev,
> +					      struct sk_buff *skb)
> +{
> +	struct hci_rp_write_auth_payload_to *rp = (void *)skb->data;
> +	struct hci_cp_write_auth_payload_to *sent;
> +
> +	BT_DBG("%s status 0x%2.2x", hdev->name, rp->status);
> +
> +	if (rp->status)
> +		return;
> +
> +	sent = hci_sent_cmd_data(hdev, HCI_OP_WRITE_AUTH_PAYLOAD_TO);
> +	if (!sent)
> +		return;
> +
> +	hci_dev_lock(hdev);
> +
> +	if (!rp->status)
> +		hdev->features[1][0] |= LMP_PING;
> +	else
> +		hdev->features[1][0] &= ~LMP_PING;

This part I do not get all. Why disable LMP_PING. And more importantly why disable the BR/EDR feature?

> +
> +	hci_dev_unlock(hdev);
> +}
> +
> static void hci_cc_read_local_commands(struct hci_dev *hdev,
> 				       struct sk_buff *skb)
> {
> @@ -3170,6 +3195,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb,
> 		hci_cc_write_sc_support(hdev, skb);
> 		break;
> 
> +	case HCI_OP_WRITE_AUTH_PAYLOAD_TO:
> +		hci_cc_write_auth_payload_timeout(hdev, skb);
> +		break;
> +
> 	case HCI_OP_READ_LOCAL_VERSION:
> 		hci_cc_read_local_version(hdev, skb);
> 		break;
> @@ -4971,7 +5000,24 @@ static void le_conn_complete_evt(struct hci_dev *hdev, u8 status,
> 			params->conn = NULL;
> 		}
> 	}
> +	/* Set the default Authenticated Payload Timeout after
> +	 * an LE Link is established. As per Core Spec v5.0, Vol 2, Part B
> +	 * Section 3.3, the HCI command WRITE_AUTH_PAYLOAD_TIMEOUT should be
> +	 * sent when the link is active and Encryption is enabled, the conn
> +	 * type must be LE and controller must support LE Ping.
> +	 */
> +	if (conn->type == LE_LINK && lmp_ping_capable(hdev) &&
> +	    test_bit(HCI_ENCRYPT, &hdev->flags)) {
> +		struct hci_cp_write_auth_payload_to cp;
> 
> +		cp.conn_handle = cpu_to_le16(conn->handle);
> +		cp.timeout = cpu_to_le16(hdev->le_auth_payload_timeout);
> +		hci_send_cmd(conn->hdev, HCI_OP_WRITE_AUTH_PAYLOAD_TO,
> +			     sizeof(cp), &cp);
> +	} else {
> +		conn->state = BT_CONNECTED;
> +		hci_connect_cfm(conn, status);
> +	}

Reading the comment above, I do not understand why are sending the command here. Have you ever tested that this works? The Connection Complete event will never result in an encrypted connection (except for BR/EDR Security Mode 3). Meaning this should be sent when the encryption state changes to enabled.

In addition, there are a bunch of requirements for the timeout value noted in the specification that we better take care of instead of sending an invalid parameter.

Regards

Marcel

[PATCH] Add Support for LE Ping feature

[SpoorthiX K]

From: Spoorthi Ravishankar Koppad <spoorthix.k@intel.com>

Changes made to add HCI Write Authenticated Payload timeout
command for LE Ping feature.
As per the Core Specification 5.0 Volume 2 Part E Section 7.3.94,
the following code changes implements
HCI Write Authenticated Payload timeout command for LE Ping feature.

Signed-off-by: Spoorthi Ravishankar Koppad <spoorthix.k@intel.com>
---
 include/net/bluetooth/hci.h      | 10 +++++++++
 include/net/bluetooth/hci_core.h |  2 ++
 net/bluetooth/hci_conn.c         |  1 +
 net/bluetooth/hci_core.c         |  1 +
 net/bluetooth/hci_debugfs.c      | 31 ++++++++++++++++++++++++++++
 net/bluetooth/hci_event.c        | 44 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 89 insertions(+)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index fbba43e..769a007 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1142,6 +1142,16 @@ struct hci_cp_write_sc_support {
 	__u8	support;
 } __packed;
 
+#define HCI_OP_WRITE_AUTH_PAYLOAD_TO    0x0c7c
+struct hci_cp_write_auth_payload_to {
+	__le16	handle;
+	__u16	timeout;
+} __packed;
+struct hci_rp_write_auth_payload_to {
+	__u8	status;
+	__le16	handle;
+} __packed;
+
 #define HCI_OP_READ_LOCAL_OOB_EXT_DATA	0x0c7d
 struct hci_rp_read_local_oob_ext_data {
 	__u8     status;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 094e61e..ce0c7a2 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -421,6 +421,7 @@ struct hci_dev {
 	__u32			rpa_timeout;
 	struct delayed_work	rpa_expired;
 	bdaddr_t		rpa;
+	__u16			auth_payload_timeout;
 
 #if IS_ENABLED(CONFIG_BT_LEDS)
 	struct led_trigger	*power_led;
@@ -500,6 +501,7 @@ struct hci_conn {
 	__u8		remote_id;
 
 	unsigned int	sent;
+	bool		auth_payload_timeout_set;
 
 	struct sk_buff_head data_q;
 	struct list_head chan_list;
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index bd4978c..cf036b8 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -516,6 +516,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst,
 	conn->rssi = HCI_RSSI_INVALID;
 	conn->tx_power = HCI_TX_POWER_INVALID;
 	conn->max_tx_power = HCI_TX_POWER_INVALID;
+	conn->auth_payload_timeout_set = false;
 
 	set_bit(HCI_CONN_POWER_SAVE, &conn->flags);
 	conn->disc_timeout = HCI_DISCONN_TIMEOUT;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index d6b2540..29f524d 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3193,6 +3193,7 @@ struct hci_dev *hci_alloc_dev(void)
 	hdev->le_max_tx_time = 0x0148;
 	hdev->le_max_rx_len = 0x001b;
 	hdev->le_max_rx_time = 0x0148;
+	hdev->auth_payload_timeout = 0x0bb8;
 	hdev->le_max_key_size = SMP_MAX_ENC_KEY_SIZE;
 	hdev->le_min_key_size = SMP_MIN_ENC_KEY_SIZE;
 	hdev->le_tx_def_phys = HCI_LE_SET_PHY_1M;
diff --git a/net/bluetooth/hci_debugfs.c b/net/bluetooth/hci_debugfs.c
index 51f5b1e..f7ab4d6 100644
--- a/net/bluetooth/hci_debugfs.c
+++ b/net/bluetooth/hci_debugfs.c
@@ -941,6 +941,35 @@ static int adv_max_interval_get(void *data, u64 *val)
 DEFINE_SIMPLE_ATTRIBUTE(adv_max_interval_fops, adv_max_interval_get,
 			adv_max_interval_set, "%llu\n");
 
+static int auth_payload_timeout_set(void *data, u64 val)
+{
+	struct hci_dev *hdev = data;
+
+	if (val < 0x0001 || val > 0xffff)
+		return -EINVAL;
+
+	hci_dev_lock(hdev);
+	hdev->auth_payload_timeout = val;
+	hci_dev_unlock(hdev);
+
+	return 0;
+}
+
+static int auth_payload_timeout_get(void *data, u64 *val)
+{
+	struct hci_dev *hdev = data;
+
+	hci_dev_lock(hdev);
+	*val = hdev->auth_payload_timeout;
+	hci_dev_unlock(hdev);
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(auth_payload_timeout_fops,
+			auth_payload_timeout_get,
+			auth_payload_timeout_set, "%llu\n");
+
 DEFINE_QUIRK_ATTRIBUTE(quirk_strict_duplicate_filter,
 		       HCI_QUIRK_STRICT_DUPLICATE_FILTER);
 DEFINE_QUIRK_ATTRIBUTE(quirk_simultaneous_discovery,
@@ -992,6 +1021,8 @@ void hci_debugfs_create_le(struct hci_dev *hdev)
 			    &adv_min_interval_fops);
 	debugfs_create_file("adv_max_interval", 0644, hdev->debugfs, hdev,
 			    &adv_max_interval_fops);
+	debugfs_create_file("auth_payload_timeout", 0644, hdev->debugfs, hdev,
+			    &auth_payload_timeout_fops);
 	debugfs_create_u16("discov_interleaved_timeout", 0644, hdev->debugfs,
 			   &hdev->discov_interleaved_timeout);
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 609fd68..94a8e58 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -564,6 +564,25 @@ static void hci_cc_read_local_version(struct hci_dev *hdev, struct sk_buff *skb)
 	}
 }
 
+static void hci_cc_write_auth_payload_timeout(struct hci_dev *hdev,
+					      struct sk_buff *skb)
+{
+	struct hci_rp_write_auth_payload_to *rp = (void *)skb->data;
+	struct hci_conn *conn;
+
+	BT_DBG("%s status 0x%2.2x", hdev->name, rp->status);
+
+	if (rp->status)
+		return;
+
+	hci_dev_lock(hdev);
+	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(rp->handle));
+
+	if (conn)
+		conn->auth_payload_timeout_set = true;
+	hci_dev_unlock(hdev);
+}
+
 static void hci_cc_read_local_commands(struct hci_dev *hdev,
 				       struct sk_buff *skb)
 {
@@ -2974,6 +2993,27 @@ static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 		goto unlock;
 	}
+	/* Set the default Authenticated Payload Timeout after
+	 * an LE Link is established. As per Core Spec v5.0, Vol 2, Part B
+	 * Section 3.3, the HCI command WRITE_AUTH_PAYLOAD_TIMEOUT should be
+	 * sent when the link is active and Encryption is enabled, the conn
+	 * type can be either LE or ACL and controller must support LMP Ping.
+	 * Ensure for AES-CCM encryption as well.
+	 */
+	if ((conn->type == LE_LINK || conn->type == ACL_LINK) &
+		lmp_ping_capable(hdev) &&
+		test_bit(HCI_CONN_ENCRYPT, &conn->flags) &&
+		test_bit(HCI_CONN_AES_CCM, &conn->flags)) {
+		struct hci_cp_write_auth_payload_to cp;
+
+		cp.handle = cpu_to_le16(conn->handle);
+		cp.timeout = cpu_to_le16(hdev->auth_payload_timeout);
+		hci_send_cmd(conn->hdev, HCI_OP_WRITE_AUTH_PAYLOAD_TO,
+			     sizeof(cp), &cp);
+	} else {
+		conn->state = BT_CONNECTED;
+		hci_connect_cfm(conn, ev->status);
+	}
 
 notify:
 	if (conn->state == BT_CONFIG) {
@@ -3170,6 +3210,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb,
 		hci_cc_write_sc_support(hdev, skb);
 		break;
 
+	case HCI_OP_WRITE_AUTH_PAYLOAD_TO:
+		hci_cc_write_auth_payload_timeout(hdev, skb);
+		break;
+
 	case HCI_OP_READ_LOCAL_VERSION:
 		hci_cc_read_local_version(hdev, skb);
 		break;
-- 
1.9.1

Re: [PATCH] Add Support for LE Ping feature

[kbuild test robot]

Hi SpoorthiX,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on v5.1-rc2 next-20190327]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/SpoorthiX-K/Add-Support-for-LE-Ping-feature/20190327-182038
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'


sparse warnings: (new ones prefixed by >>)

>> net/bluetooth/hci_event.c:3010:28: sparse: incorrect type in assignment (different base types) @@    expected unsigned short [unsigned] [assigned] [usertype] timeout @@    got igned] [assigned] [usertype] timeout @@
   net/bluetooth/hci_event.c:3010:28:    expected unsigned short [unsigned] [assigned] [usertype] timeout
   net/bluetooth/hci_event.c:3010:28:    got restricted __le16 [usertype] <noident>
   include/linux/overflow.h:285:13: sparse: undefined identifier '__builtin_mul_overflow'
   include/linux/overflow.h:285:13: sparse: incorrect type in conditional
   include/linux/overflow.h:285:13:    got void
   include/linux/overflow.h:287:13: sparse: undefined identifier '__builtin_add_overflow'
   include/linux/overflow.h:287:13: sparse: incorrect type in conditional
   include/linux/overflow.h:287:13:    got void
   include/linux/overflow.h:285:13: sparse: not a function <noident>
   include/linux/overflow.h:285:13: sparse: incorrect type in conditional
   include/linux/overflow.h:285:13:    got void
   include/linux/overflow.h:287:13: sparse: not a function <noident>
   include/linux/overflow.h:287:13: sparse: incorrect type in conditional
   include/linux/overflow.h:287:13:    got void
   include/linux/overflow.h:285:13: sparse: call with no type!
   include/linux/overflow.h:287:13: sparse: call with no type!

vim +3010 net/bluetooth/hci_event.c

  2904	
  2905	static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
  2906	{
  2907		struct hci_ev_encrypt_change *ev = (void *) skb->data;
  2908		struct hci_conn *conn;
  2909	
  2910		BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
  2911	
  2912		hci_dev_lock(hdev);
  2913	
  2914		conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(ev->handle));
  2915		if (!conn)
  2916			goto unlock;
  2917	
  2918		if (!ev->status) {
  2919			if (ev->encrypt) {
  2920				/* Encryption implies authentication */
  2921				set_bit(HCI_CONN_AUTH, &conn->flags);
  2922				set_bit(HCI_CONN_ENCRYPT, &conn->flags);
  2923				conn->sec_level = conn->pending_sec_level;
  2924	
  2925				/* P-256 authentication key implies FIPS */
  2926				if (conn->key_type == HCI_LK_AUTH_COMBINATION_P256)
  2927					set_bit(HCI_CONN_FIPS, &conn->flags);
  2928	
  2929				if ((conn->type == ACL_LINK && ev->encrypt == 0x02) ||
  2930				    conn->type == LE_LINK)
  2931					set_bit(HCI_CONN_AES_CCM, &conn->flags);
  2932			} else {
  2933				clear_bit(HCI_CONN_ENCRYPT, &conn->flags);
  2934				clear_bit(HCI_CONN_AES_CCM, &conn->flags);
  2935			}
  2936		}
  2937	
  2938		/* We should disregard the current RPA and generate a new one
  2939		 * whenever the encryption procedure fails.
  2940		 */
  2941		if (ev->status && conn->type == LE_LINK) {
  2942			hci_dev_set_flag(hdev, HCI_RPA_EXPIRED);
  2943			hci_adv_instances_set_rpa_expired(hdev, true);
  2944		}
  2945	
  2946		clear_bit(HCI_CONN_ENCRYPT_PEND, &conn->flags);
  2947	
  2948		if (ev->status && conn->state == BT_CONNECTED) {
  2949			if (ev->status == HCI_ERROR_PIN_OR_KEY_MISSING)
  2950				set_bit(HCI_CONN_AUTH_FAILURE, &conn->flags);
  2951	
  2952			hci_disconnect(conn, HCI_ERROR_AUTH_FAILURE);
  2953			hci_conn_drop(conn);
  2954			goto unlock;
  2955		}
  2956	
  2957		/* In Secure Connections Only mode, do not allow any connections
  2958		 * that are not encrypted with AES-CCM using a P-256 authenticated
  2959		 * combination key.
  2960		 */
  2961		if (hci_dev_test_flag(hdev, HCI_SC_ONLY) &&
  2962		    (!test_bit(HCI_CONN_AES_CCM, &conn->flags) ||
  2963		     conn->key_type != HCI_LK_AUTH_COMBINATION_P256)) {
  2964			hci_connect_cfm(conn, HCI_ERROR_AUTH_FAILURE);
  2965			hci_conn_drop(conn);
  2966			goto unlock;
  2967		}
  2968	
  2969		/* Try reading the encryption key size for encrypted ACL links */
  2970		if (!ev->status && ev->encrypt && conn->type == ACL_LINK) {
  2971			struct hci_cp_read_enc_key_size cp;
  2972			struct hci_request req;
  2973	
  2974			/* Only send HCI_Read_Encryption_Key_Size if the
  2975			 * controller really supports it. If it doesn't, assume
  2976			 * the default size (16).
  2977			 */
  2978			if (!(hdev->commands[20] & 0x10)) {
  2979				conn->enc_key_size = HCI_LINK_KEY_SIZE;
  2980				goto notify;
  2981			}
  2982	
  2983			hci_req_init(&req, hdev);
  2984	
  2985			cp.handle = cpu_to_le16(conn->handle);
  2986			hci_req_add(&req, HCI_OP_READ_ENC_KEY_SIZE, sizeof(cp), &cp);
  2987	
  2988			if (hci_req_run_skb(&req, read_enc_key_size_complete)) {
  2989				bt_dev_err(hdev, "sending read key size failed");
  2990				conn->enc_key_size = HCI_LINK_KEY_SIZE;
  2991				goto notify;
  2992			}
  2993	
  2994			goto unlock;
  2995		}
  2996		/* Set the default Authenticated Payload Timeout after
  2997		 * an LE Link is established. As per Core Spec v5.0, Vol 2, Part B
  2998		 * Section 3.3, the HCI command WRITE_AUTH_PAYLOAD_TIMEOUT should be
  2999		 * sent when the link is active and Encryption is enabled, the conn
  3000		 * type can be either LE or ACL and controller must support LMP Ping.
  3001		 * Ensure for AES-CCM encryption as well.
  3002		 */
  3003		if ((conn->type == LE_LINK || conn->type == ACL_LINK) &
  3004			lmp_ping_capable(hdev) &&
  3005			test_bit(HCI_CONN_ENCRYPT, &conn->flags) &&
  3006			test_bit(HCI_CONN_AES_CCM, &conn->flags)) {
  3007			struct hci_cp_write_auth_payload_to cp;
  3008	
  3009			cp.handle = cpu_to_le16(conn->handle);
> 3010			cp.timeout = cpu_to_le16(hdev->auth_payload_timeout);
  3011			hci_send_cmd(conn->hdev, HCI_OP_WRITE_AUTH_PAYLOAD_TO,
  3012				     sizeof(cp), &cp);
  3013		} else {
  3014			conn->state = BT_CONNECTED;
  3015			hci_connect_cfm(conn, ev->status);
  3016		}
  3017	
  3018	notify:
  3019		if (conn->state == BT_CONFIG) {
  3020			if (!ev->status)
  3021				conn->state = BT_CONNECTED;
  3022	
  3023			hci_connect_cfm(conn, ev->status);
  3024			hci_conn_drop(conn);
  3025		} else
  3026			hci_encrypt_cfm(conn, ev->status, ev->encrypt);
  3027	
  3028	unlock:
  3029		hci_dev_unlock(hdev);
  3030	}
  3031	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH] Add Support for LE Ping feature

[Johan Hedberg]

Hi Spoorthi,

On 27 Mar 2019, at 5.43, SpoorthiX K <spoorthix.k@intel.com> wrote:
> +#define HCI_OP_WRITE_AUTH_PAYLOAD_TO    0x0c7c
> +struct hci_cp_write_auth_payload_to {
> +	__le16	handle;
> +	__u16	timeout;

As Marcel (and now also the build test robot) pointed out this should be __le16.

> #if IS_ENABLED(CONFIG_BT_LEDS)
> 	struct led_trigger	*power_led;
> @@ -500,6 +501,7 @@ struct hci_conn {
> 	__u8		remote_id;
> 
> 	unsigned int	sent;
> +	bool		auth_payload_timeout_set;

Would it make sense to add this to the flags instead of increasing the hci_conn struct size?

> +	/* Set the default Authenticated Payload Timeout after
> +	 * an LE Link is established.

The test further below also includes BR/EDR links, i.e. the above comment doesn’t match that.

> +	 * Section 3.3, the HCI command WRITE_AUTH_PAYLOAD_TIMEOUT should be
> +	 * sent when the link is active and Encryption is enabled, the conn
> +	 * type can be either LE or ACL and controller must support LMP Ping.
> +	 * Ensure for AES-CCM encryption as well.
> +	 */
> +	if ((conn->type == LE_LINK || conn->type == ACL_LINK) &

That ‘&’ the end should probably be a ‘&&’, shouldn’t it?

> +	} else {
> +		conn->state = BT_CONNECTED;
> +		hci_connect_cfm(conn, ev->status);
> +	}

Why is this being added? Such a transition to BT_CONNECTED state at this point in the code didn’t exist previously, and this doesn’t seem to be related to the new feature.

Johan