Re: [PATCH v3 5/5] Bluetooth: Fallback transparent SCO from T2 to T1

[Marcel Holtmann <marcel@holtmann.org>]

Hi Fred,

> When initiating a transparent eSCO connection, make use of T2 settings at
> first try. T2 is the recommended settings from HFP 1.6 WideBand Speech. Upon
> connection failure, try T1 settings.
> To know which of T2 or T1 should be used, the connection attempt index is used.
> T2 failure is detected if Synchronous Connection Complete Event fails with
> error 0x0d. This error code has been found experimentally by sending a T2
> request to a T1 only SCO listener. It means "Connection Rejected due to
> Limited resource".
> 
> Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
> ---
>  net/bluetooth/hci_conn.c  |   11 ++++++++++-
>  net/bluetooth/hci_event.c |    1 +
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 504367e..a3f4e29 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -179,12 +179,21 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
>  	cp.tx_bandwidth   = __constant_cpu_to_le32(0x00001f40);
>  	cp.rx_bandwidth   = __constant_cpu_to_le32(0x00001f40);
>  
> -	if (test_and_clear_bit(HCI_CONN_SCO_TRANSPARENT, &conn->flags)) {
> +	if (conn->attempt == 1 &&
> +	    test_bit(HCI_CONN_SCO_TRANSPARENT, &conn->flags)) {
>  		cp.pkt_type       = __constant_cpu_to_le16(EDR_ESCO_MASK &
>  							   ~ESCO_2EV3);
>  		cp.max_latency    = __constant_cpu_to_le16(0x000d);
>  		cp.voice_setting  = __constant_cpu_to_le16(0x0003);
>  		cp.retrans_effort = 0x02;
> +	} else if (conn->attempt == 2 &&
> +		   test_bit(HCI_CONN_SCO_TRANSPARENT, &conn->flags)) {

I do not really like abusing a simple counter as a state machine. We
should do something more explicit with tracking if T2 vs T1 sco_flags or
something.

> +		cp.pkt_type       = __constant_cpu_to_le16(EDR_ESCO_MASK |
> +							   ESCO_EV3);
> +		cp.max_latency    = __constant_cpu_to_le16(0x0007);
> +		cp.voice_setting  = __constant_cpu_to_le16(0x0003);
> +		cp.retrans_effort = 0x02;
> +		clear_bit(HCI_CONN_SCO_TRANSPARENT, &conn->flags);

If we clear this here, does it mean we end up creating a CVSD link. That
can not be correct. I think we need to fail the connection procedure in
that case. We can not magically turn a transparent link into a CVSD
link. That would confuse the hell out of HFP.

>  	} else {
>  		cp.pkt_type       = cpu_to_le16(conn->pkt_type);
>  		cp.max_latency    = __constant_cpu_to_le16(0xffff);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 3fd0212..6c1aec9 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3330,6 +3330,7 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
>  		hci_conn_add_sysfs(conn);
>  		break;
>  
> +	case 0x0d:	/* No resource available */
>  	case 0x11:	/* Unsupported Feature or Parameter Value */
>  	case 0x1c:	/* SCO interval rejected */
>  	case 0x1a:	/* Unsupported Remote Feature */

Regards

Marcel