Re: [PATCH v2] Bluetooth: hci_event: Add HCI_Write_Link_Supervision_Timeout command/event structures

Dear Chen,


Thank you for your patch.

Am 24.07.26 um 10:54 schrieb [email protected]:
> From: Chen Zhang <[email protected]>
> 
> Some speakers do not actively initiate disconnection when power off. As

… powering …

> a result, the center can only wait for a timeout to disconnect, with the

What does “the center” refer to?

> default timeout being 20s. In certain scenarios, this can significantly
> impact user experience. For example, if a speaker is playing music and
> is then turned off, it takes 20s before the music starts playing from
> the center.
> 
> Define HCI_Write_Link_Supervision_Timeout command, event structure, and
> corresponding event handler function, set the timeout to 5s after ACL
> link is established.

Why five seconds and not less? How do Chromium OS or Android do it?

Please also document one test setup up, where you could reproduce it.

> Signed-off-by: Chen Zhang <[email protected]>
> ---
>   include/net/bluetooth/hci.h      | 10 +++++++
>   include/net/bluetooth/hci_core.h |  1 +
>   net/bluetooth/hci_event.c        | 50 ++++++++++++++++++++++++++++++++
>   3 files changed, 61 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 50f0eef71fb1..18a5a17eb218 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1239,6 +1239,16 @@ struct hci_cp_host_buffer_size {
>   	__le16   sco_max_pkt;
>   } __packed;
>   
> +#define HCI_OP_WRITE_LINK_SUPERVISION_TIMEOUT	0x0c37
> +struct hci_cp_write_link_supervision_timeout {
> +	__le16	 handle;
> +	__le16	 timeout;

How about appending the unit for the timeout: timeout_s.

> +} __packed;
> +struct hci_rp_write_link_supervision_timeout {
> +	__u8	 status;
> +	__le16	 handle;
> +} __packed;
> +
>   #define HCI_OP_READ_NUM_SUPPORTED_IAC	0x0c38
>   struct hci_rp_read_num_supported_iac {
>   	__u8	status;
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index e7133ff87fbf..866903a9b5c8 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -702,6 +702,7 @@ struct hci_conn {
>   	__u8		le_features[248];
>   	__u16		pkt_type;
>   	__u16		link_policy;
> +	__u16		link_supervision_timeout;

Ditto.

>   	__u8		key_type;
>   	__u8		auth_type;
>   	__u8		sec_level;
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 741d658e9630..653184d0dfc7 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -606,6 +606,44 @@ static u8 hci_cc_write_voice_setting(struct hci_dev *hdev, void *data,
>   	return rp->status;
>   }
>   
> +static u8 hci_cc_write_link_supervision_timeout(struct hci_dev *hdev, void *data,
> +						struct sk_buff *skb)
> +{
> +	struct hci_ev_status *rp = data;
> +	struct hci_cp_write_link_supervision_timeout *sent;
> +	struct hci_conn *conn;
> +	u16 handle;
> +	u8 status = rp->status;
> +
> +	bt_dev_dbg(hdev, "status 0x%2.2x", status);
> +
> +	sent = hci_sent_cmd_data(hdev, HCI_OP_WRITE_LINK_SUPERVISION_TIMEOUT);
> +	if (!sent)
> +		return status;
> +
> +	handle = le16_to_cpu(sent->handle);
> +
> +	hci_dev_lock(hdev);
> +
> +	conn = hci_conn_hash_lookup_handle(hdev, handle);
> +	if (!conn) {
> +		status = 0xFF;
> +		goto done;
> +	}
> +
> +	if (!status) {
> +		conn->link_supervision_timeout = __le16_to_cpu(sent->timeout);
> +		bt_dev_dbg(hdev, "handle 0x%4.4x timeout set to 0x%4.4x (%u ms)",
> +			   handle, conn->link_supervision_timeout,
> +			   conn->link_supervision_timeout * 5 / 8);
> +	}
> +
> +done:
> +	hci_dev_unlock(hdev);
> +
> +	return status;
> +}
> +
>   static u8 hci_cc_read_num_supported_iac(struct hci_dev *hdev, void *data,
>   					struct sk_buff *skb)
>   {
> @@ -3240,6 +3278,16 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
>   			hci_send_cmd(hdev, HCI_OP_CHANGE_CONN_PTYPE, sizeof(cp),
>   				     &cp);
>   		}
> +
> +		if (conn->type == ACL_LINK && conn->role == HCI_ROLE_MASTER) {
> +			struct hci_cp_write_link_supervision_timeout cp;
> +
> +			cp.handle = ev->handle;
> +			cp.timeout = 0x1F40;	/* 8000 * 0.625ms = 5000ms */
> +			hci_send_cmd(hdev, HCI_OP_WRITE_LINK_SUPERVISION_TIMEOUT,
> +				     sizeof(cp), &cp);
> +		}
> +
>   	}
>   
>   	if (conn->type == ACL_LINK)
> @@ -4097,6 +4145,8 @@ static const struct hci_cc {
>   	HCI_CC_STATUS(HCI_OP_WRITE_CLASS_OF_DEV, hci_cc_write_class_of_dev),
>   	HCI_CC(HCI_OP_READ_VOICE_SETTING, hci_cc_read_voice_setting,
>   	       sizeof(struct hci_rp_read_voice_setting)),
> +	HCI_CC(HCI_OP_WRITE_LINK_SUPERVISION_TIMEOUT, hci_cc_write_link_supervision_timeout,
> +	       sizeof(struct hci_rp_write_link_supervision_timeout)),
>   	HCI_CC_STATUS(HCI_OP_WRITE_VOICE_SETTING, hci_cc_write_voice_setting),
>   	HCI_CC(HCI_OP_READ_NUM_SUPPORTED_IAC, hci_cc_read_num_supported_iac,
>   	       sizeof(struct hci_rp_read_num_supported_iac)),


Kind regards,

Paul