2019-03-18 10:29:21

by K, SpoorthiX

[permalink] [raw]
Subject: [PATCH] Add support for LE ping feature

From: SpoorthiX <[email protected]>

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 <[email protected]>
---
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



2019-03-18 17:02:45

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] Add support for LE ping feature

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 <[email protected]>

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


2019-03-18 21:11:34

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH] Add support for LE ping feature

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