Re: [PATCH] Add support for LE ping feature

[Marcel Holtmann <marcel@holtmann.org>]

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