[PATCH 1/1] bluetooth: Check for SCO type before setting retransmission effort

[PATCH 1/1] bluetooth: Check for SCO type before setting retransmission effort

[Bernhard Thaler]

From: "Bernhard Thaler" <bernhard.thaler@r-it.at>

SCO connection cannot be setup to devices that do not support retransmission.
Patch based on http://permalink.gmane.org/gmane.linux.bluez.kernel/7779 and
adapted for this kernel version.
Code changed to check SCO/eSCO type before setting retransmission effort 
and max. latency. The purpose of the patch is to support older devices not
 capable of eSCO.

Tested on Blackberry 655+ headset which does not support retransmission.
Credits go to Alexander Sommerhuber.

Signed-off-by: Bernhard Thaler <bernhard.thaler@r-it.at>
---
 net/bluetooth/hci_conn.c |   39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index faff624..093abcc 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -186,26 +186,31 @@ bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
 	cp.rx_bandwidth   = cpu_to_le32(0x00001f40);
 	cp.voice_setting  = cpu_to_le16(conn->setting);
 
-	switch (conn->setting & SCO_AIRMODE_MASK) {
-	case SCO_AIRMODE_TRANSP:
-		if (conn->attempt > ARRAY_SIZE(sco_param_wideband))
-			return false;
-		cp.retrans_effort = 0x02;
-		param = &sco_param_wideband[conn->attempt - 1];
-		break;
-	case SCO_AIRMODE_CVSD:
-		if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
+	if (!(conn->link->features[0][3] & LMP_ESCO)) {
+		cp.retrans_effort = 0xff;
+		cp.pkt_type = __cpu_to_le16(conn->pkt_type);
+		cp.max_latency = __cpu_to_le16(0xffff);
+	} else {
+		switch (conn->setting & SCO_AIRMODE_MASK) {
+		case SCO_AIRMODE_TRANSP:
+			if (conn->attempt > ARRAY_SIZE(sco_param_wideband))
+				return false;
+			cp.retrans_effort = 0x02;
+			param = &sco_param_wideband[conn->attempt - 1];
+			break;
+		case SCO_AIRMODE_CVSD:
+			if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
+				return false;
+			cp.retrans_effort = 0x01;
+			param = &sco_param_cvsd[conn->attempt - 1];
+			break;
+		default:
 			return false;
-		cp.retrans_effort = 0x01;
-		param = &sco_param_cvsd[conn->attempt - 1];
-		break;
-	default:
-		return false;
+		}
+		cp.pkt_type = __cpu_to_le16(param->pkt_type);
+		cp.max_latency = __cpu_to_le16(param->max_latency);
 	}
 
-	cp.pkt_type = __cpu_to_le16(param->pkt_type);
-	cp.max_latency = __cpu_to_le16(param->max_latency);
-
 	if (hci_send_cmd(hdev, HCI_OP_SETUP_SYNC_CONN, sizeof(cp), &cp) < 0)
 		return false;
 
-- 
1.7.10.4

Re: [PATCH 1/1] bluetooth: Check for SCO type before setting retransmission effort

[Marcel Holtmann]

Hi Bernhard,

> SCO connection cannot be setup to devices that do not support retransmission.
> Patch based on http://permalink.gmane.org/gmane.linux.bluez.kernel/7779 and
> adapted for this kernel version.
> Code changed to check SCO/eSCO type before setting retransmission effort 
> and max. latency. The purpose of the patch is to support older devices not
> capable of eSCO.
> 
> Tested on Blackberry 655+ headset which does not support retransmission.
> Credits go to Alexander Sommerhuber.
> 
> Signed-off-by: Bernhard Thaler <bernhard.thaler@r-it.at>
> ---
> net/bluetooth/hci_conn.c |   39 ++++++++++++++++++++++-----------------
> 1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index faff624..093abcc 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -186,26 +186,31 @@ bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
> 	cp.rx_bandwidth   = cpu_to_le32(0x00001f40);
> 	cp.voice_setting  = cpu_to_le16(conn->setting);
> 
> -	switch (conn->setting & SCO_AIRMODE_MASK) {
> -	case SCO_AIRMODE_TRANSP:
> -		if (conn->attempt > ARRAY_SIZE(sco_param_wideband))
> -			return false;
> -		cp.retrans_effort = 0x02;
> -		param = &sco_param_wideband[conn->attempt - 1];
> -		break;
> -	case SCO_AIRMODE_CVSD:
> -		if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
> +	if (!(conn->link->features[0][3] & LMP_ESCO)) {

so we have something that is called lmp_esco_capable.

> +		cp.retrans_effort = 0xff;
> +		cp.pkt_type = __cpu_to_le16(conn->pkt_type);
> +		cp.max_latency = __cpu_to_le16(0xffff);

Anyway, I do not thing this is the right approach. I think what we should do is just reject a connection attempt for transparent eSCO connections (since we only use them for wideband speech anyway) when connecting to a device that has no eSCO support.

> +	} else {
> +		switch (conn->setting & SCO_AIRMODE_MASK) {
> +		case SCO_AIRMODE_TRANSP:
> +			if (conn->attempt > ARRAY_SIZE(sco_param_wideband))
> +				return false;
> +			cp.retrans_effort = 0x02;
> +			param = &sco_param_wideband[conn->attempt - 1];
> +			break;
> +		case SCO_AIRMODE_CVSD:
> +			if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
> +				return false;
> +			cp.retrans_effort = 0x01;
> +			param = &sco_param_cvsd[conn->attempt - 1];
> +			break;

For CVSD based attempts, we should start with non eSCO parameter settings if we know that the remote side can not support eSCO.

The reason is that you still need to follow through with defined parameter sets from the HFP profile.

Regards

Marcel

[PATCHv2 1/1] bluetooth: Check for SCO type before setting retransmission effort

[Bernhard Thaler]

SCO connection cannot be setup to devices that do not support retransmission.
Patch based on http://permalink.gmane.org/gmane.linux.bluez.kernel/7779 and
adapted for this kernel version.
Code changed to check SCO/eSCO type before setting retransmission effort
and max. latency. The purpose of the patch is to support older devices not
 capable of eSCO.

Tested on Blackberry 655+ headset which does not support retransmission.
Credits go to Alexander Sommerhuber.

Signed-off-by: Bernhard Thaler <bernhard.thaler@r-it.at>
---
 net/bluetooth/hci_conn.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index faff624..f6e3183 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -194,10 +194,18 @@ bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
 		param = &sco_param_wideband[conn->attempt - 1];
 		break;
 	case SCO_AIRMODE_CVSD:
-		if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
-			return false;
-		cp.retrans_effort = 0x01;
-		param = &sco_param_cvsd[conn->attempt - 1];
+		if (!lmp_esco_capable(conn->link)) {
+			/* sco_param_cvsd contains SCO in last two elements */
+			if ((conn->attempt + 3) > ARRAY_SIZE(sco_param_cvsd))
+				return false;
+			cp.retrans_effort = 0xff;
+			param = &sco_param_cvsd[(conn->attempt + 2)];
+		} else {
+			if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
+				return false;
+			cp.retrans_effort = 0x01;
+			param = &sco_param_cvsd[conn->attempt - 1];
+		}
 		break;
 	default:
 		return false;
-- 
1.7.10.4

Re: [PATCHv2 1/1] bluetooth: Check for SCO type before setting retransmission effort

[Marcel Holtmann]

Hi Bernhard,

> SCO connection cannot be setup to devices that do not support retransmission.
> Patch based on http://permalink.gmane.org/gmane.linux.bluez.kernel/7779 and
> adapted for this kernel version.
> Code changed to check SCO/eSCO type before setting retransmission effort
> and max. latency. The purpose of the patch is to support older devices not
> capable of eSCO.
> 
> Tested on Blackberry 655+ headset which does not support retransmission.
> Credits go to Alexander Sommerhuber.
> 
> Signed-off-by: Bernhard Thaler <bernhard.thaler@r-it.at>
> ---
> net/bluetooth/hci_conn.c |   16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index faff624..f6e3183 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -194,10 +194,18 @@ bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
> 		param = &sco_param_wideband[conn->attempt - 1];
> 		break;
> 	case SCO_AIRMODE_CVSD:
> -		if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
> -			return false;
> -		cp.retrans_effort = 0x01;
> -		param = &sco_param_cvsd[conn->attempt - 1];
> +		if (!lmp_esco_capable(conn->link)) {
> +			/* sco_param_cvsd contains SCO in last two elements */
> +			if ((conn->attempt + 3) > ARRAY_SIZE(sco_param_cvsd))
> +				return false;
> +			cp.retrans_effort = 0xff;
> +			param = &sco_param_cvsd[(conn->attempt + 2)];

so if anybody creates a new mode and forgets to update this code, we are hitting a nice crash and/or wrong behavior.

I would prefer that we create separate parameter arrays here. It might means that we have to rename the current ones to esco_param_* and the introduce sco_param_cvsd.

> +		} else {
> +			if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
> +				return false;
> +			cp.retrans_effort = 0x01;
> +			param = &sco_param_cvsd[conn->attempt - 1];
> +		}
> 		break;
> 	default:
> 		return false;

Regards

Marcel

Re: [PATCHv2 1/1] bluetooth: Check for SCO type before setting retransmission effort

[Marcel Holtmann]

Hi Bernhard,

> SCO connection cannot be setup to devices that do not support retransmission.
> Patch based on http://permalink.gmane.org/gmane.linux.bluez.kernel/7779 and
> adapted for this kernel version.
> Code changed to check SCO/eSCO type before setting retransmission effort
> and max. latency. The purpose of the patch is to support older devices not
> capable of eSCO.
> 
> Tested on Blackberry 655+ headset which does not support retransmission.
> Credits go to Alexander Sommerhuber.
> 
> Signed-off-by: Bernhard Thaler <bernhard.thaler@r-it.at>
> ---
> net/bluetooth/hci_conn.c |   16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index faff624..f6e3183 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -194,10 +194,18 @@ bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
> 		param = &sco_param_wideband[conn->attempt - 1];
> 		break;
> 	case SCO_AIRMODE_CVSD:
> -		if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
> -			return false;
> -		cp.retrans_effort = 0x01;
> -		param = &sco_param_cvsd[conn->attempt - 1];
> +		if (!lmp_esco_capable(conn->link)) {
> +			/* sco_param_cvsd contains SCO in last two elements */
> +			if ((conn->attempt + 3) > ARRAY_SIZE(sco_param_cvsd))
> +				return false;
> +			cp.retrans_effort = 0xff;
> +			param = &sco_param_cvsd[(conn->attempt + 2)];

so if anybody creates a new mode and forgets to update this code, we are hitting a nice crash and/or wrong behavior.

I would prefer that we create separate parameter arrays here. It might means that we have to rename the current ones to esco_param_* and the introduce sco_param_cvsd.

> +		} else {
> +			if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
> +				return false;
> +			cp.retrans_effort = 0x01;
> +			param = &sco_param_cvsd[conn->attempt - 1];
> +		}
> 		break;
> 	default:
> 		return false;

Regards

Marcel

[PATCHv3 1/1] bluetooth: Check for SCO type before setting retransmission effort

[Bernhard Thaler]

SCO connection cannot be setup to devices that do not support retransmission.
Patch based on http://permalink.gmane.org/gmane.linux.bluez.kernel/7779 and
adapted for this kernel version.
Code changed to check SCO/eSCO type before setting retransmission effort
and max. latency. The purpose of the patch is to support older devices not
 capable of eSCO.

Tested on Blackberry 655+ headset which does not support retransmission.
Credits go to Alexander Sommerhuber.

Signed-off-by: Bernhard Thaler <bernhard.thaler@r-it.at>
---
 net/bluetooth/hci_conn.c |   22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index faff624..32dcde1 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -38,7 +38,7 @@ struct sco_param {
 	u16 max_latency;
 };
 
-static const struct sco_param sco_param_cvsd[] = {
+static const struct sco_param esco_param_cvsd[] = {
 	{ EDR_ESCO_MASK & ~ESCO_2EV3, 0x000a }, /* S3 */
 	{ EDR_ESCO_MASK & ~ESCO_2EV3, 0x0007 }, /* S2 */
 	{ EDR_ESCO_MASK | ESCO_EV3,   0x0007 }, /* S1 */
@@ -46,6 +46,11 @@ static const struct sco_param sco_param_cvsd[] = {
 	{ EDR_ESCO_MASK | ESCO_HV1,   0xffff }, /* D0 */
 };
 
+static const struct sco_param sco_param_cvsd[] = {
+	{ EDR_ESCO_MASK | ESCO_HV3,   0xffff }, /* D1 */
+	{ EDR_ESCO_MASK | ESCO_HV1,   0xffff }, /* D0 */
+};
+
 static const struct sco_param sco_param_wideband[] = {
 	{ EDR_ESCO_MASK & ~ESCO_2EV3, 0x000d }, /* T2 */
 	{ EDR_ESCO_MASK | ESCO_EV3,   0x0008 }, /* T1 */
@@ -194,10 +199,17 @@ bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
 		param = &sco_param_wideband[conn->attempt - 1];
 		break;
 	case SCO_AIRMODE_CVSD:
-		if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
-			return false;
-		cp.retrans_effort = 0x01;
-		param = &sco_param_cvsd[conn->attempt - 1];
+		if (!lmp_esco_capable(conn->link)) {
+			if ((conn->attempt) > ARRAY_SIZE(sco_param_cvsd))
+				return false;
+			cp.retrans_effort = 0xff;
+			param = &sco_param_cvsd[conn->attempt - 1];
+		} else {
+			if (conn->attempt > ARRAY_SIZE(esco_param_cvsd))
+				return false;
+			cp.retrans_effort = 0x01;
+			param = &esco_param_cvsd[conn->attempt - 1];
+		}
 		break;
 	default:
 		return false;
-- 
1.7.10.4

Re: [PATCHv3 1/1] bluetooth: Check for SCO type before setting retransmission effort

[Marcel Holtmann]

Hi Bernhard,

> SCO connection cannot be setup to devices that do not support retransmission.
> Patch based on http://permalink.gmane.org/gmane.linux.bluez.kernel/7779 and
> adapted for this kernel version.
> Code changed to check SCO/eSCO type before setting retransmission effort
> and max. latency. The purpose of the patch is to support older devices not
> capable of eSCO.
> 
> Tested on Blackberry 655+ headset which does not support retransmission.
> Credits go to Alexander Sommerhuber.
> 
> Signed-off-by: Bernhard Thaler <bernhard.thaler@r-it.at>
> ---
> net/bluetooth/hci_conn.c |   22 +++++++++++++++++-----
> 1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index faff624..32dcde1 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -38,7 +38,7 @@ struct sco_param {
> 	u16 max_latency;
> };
> 
> -static const struct sco_param sco_param_cvsd[] = {
> +static const struct sco_param esco_param_cvsd[] = {
> 	{ EDR_ESCO_MASK & ~ESCO_2EV3, 0x000a }, /* S3 */
> 	{ EDR_ESCO_MASK & ~ESCO_2EV3, 0x0007 }, /* S2 */
> 	{ EDR_ESCO_MASK | ESCO_EV3,   0x0007 }, /* S1 */
> @@ -46,6 +46,11 @@ static const struct sco_param sco_param_cvsd[] = {
> 	{ EDR_ESCO_MASK | ESCO_HV1,   0xffff }, /* D0 */
> };
> 
> +static const struct sco_param sco_param_cvsd[] = {
> +	{ EDR_ESCO_MASK | ESCO_HV3,   0xffff }, /* D1 */
> +	{ EDR_ESCO_MASK | ESCO_HV1,   0xffff }, /* D0 */
> +};
> +
> static const struct sco_param sco_param_wideband[] = {
> 	{ EDR_ESCO_MASK & ~ESCO_2EV3, 0x000d }, /* T2 */
> 	{ EDR_ESCO_MASK | ESCO_EV3,   0x0008 }, /* T1 */
> @@ -194,10 +199,17 @@ bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
> 		param = &sco_param_wideband[conn->attempt - 1];
> 		break;
> 	case SCO_AIRMODE_CVSD:
> -		if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
> -			return false;
> -		cp.retrans_effort = 0x01;
> -		param = &sco_param_cvsd[conn->attempt - 1];
> +		if (!lmp_esco_capable(conn->link)) {

please change this statement around to check if eSCO support exists and not if it does not exist. The negation is pointless if you have an else branch anyway.

> +			if ((conn->attempt) > ARRAY_SIZE(sco_param_cvsd))

The extra (conn->attempt) is not needed. Just do (conn->attempt > ARRAY_SIZE(..))

> +				return false;
> +			cp.retrans_effort = 0xff;
> +			param = &sco_param_cvsd[conn->attempt - 1];
> +		} else {
> +			if (conn->attempt > ARRAY_SIZE(esco_param_cvsd))
> +				return false;
> +			cp.retrans_effort = 0x01;
> +			param = &esco_param_cvsd[conn->attempt - 1];
> +		}
> 		break;
> 	default:
> 		return false;

The rest looks fine.

Regards

Marcel

[PATCHv4 1/1] bluetooth: Check for SCO type before setting retransmission effort

[Bernhard Thaler]

SCO connection cannot be setup to devices that do not support retransmission.
Patch based on http://permalink.gmane.org/gmane.linux.bluez.kernel/7779 and
adapted for this kernel version.
Code changed to check SCO/eSCO type before setting retransmission effort
and max. latency. The purpose of the patch is to support older devices not
 capable of eSCO.

Tested on Blackberry 655+ headset which does not support retransmission.
Credits go to Alexander Sommerhuber.

Signed-off-by: Bernhard Thaler <bernhard.thaler@r-it.at>
---
 net/bluetooth/hci_conn.c |   22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index faff624..afa325b 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -38,7 +38,7 @@ struct sco_param {
 	u16 max_latency;
 };
 
-static const struct sco_param sco_param_cvsd[] = {
+static const struct sco_param esco_param_cvsd[] = {
 	{ EDR_ESCO_MASK & ~ESCO_2EV3, 0x000a }, /* S3 */
 	{ EDR_ESCO_MASK & ~ESCO_2EV3, 0x0007 }, /* S2 */
 	{ EDR_ESCO_MASK | ESCO_EV3,   0x0007 }, /* S1 */
@@ -46,6 +46,11 @@ static const struct sco_param sco_param_cvsd[] = {
 	{ EDR_ESCO_MASK | ESCO_HV1,   0xffff }, /* D0 */
 };
 
+static const struct sco_param sco_param_cvsd[] = {
+	{ EDR_ESCO_MASK | ESCO_HV3,   0xffff }, /* D1 */
+	{ EDR_ESCO_MASK | ESCO_HV1,   0xffff }, /* D0 */
+};
+
 static const struct sco_param sco_param_wideband[] = {
 	{ EDR_ESCO_MASK & ~ESCO_2EV3, 0x000d }, /* T2 */
 	{ EDR_ESCO_MASK | ESCO_EV3,   0x0008 }, /* T1 */
@@ -194,10 +199,17 @@ bool hci_setup_sync(struct hci_conn *conn, __u16 handle)
 		param = &sco_param_wideband[conn->attempt - 1];
 		break;
 	case SCO_AIRMODE_CVSD:
-		if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
-			return false;
-		cp.retrans_effort = 0x01;
-		param = &sco_param_cvsd[conn->attempt - 1];
+		if (lmp_esco_capable(conn->link)) {
+			if (conn->attempt > ARRAY_SIZE(esco_param_cvsd))
+				return false;
+			cp.retrans_effort = 0x01;
+			param = &esco_param_cvsd[conn->attempt - 1];
+		} else {
+			if (conn->attempt > ARRAY_SIZE(sco_param_cvsd))
+				return false;
+			cp.retrans_effort = 0xff;
+			param = &sco_param_cvsd[conn->attempt - 1];
+		}
 		break;
 	default:
 		return false;
-- 
1.7.10.4

Re: [PATCHv4 1/1] bluetooth: Check for SCO type before setting retransmission effort

[Marcel Holtmann]

Hi Bernhard,

> SCO connection cannot be setup to devices that do not support retransmission.
> Patch based on http://permalink.gmane.org/gmane.linux.bluez.kernel/7779 and
> adapted for this kernel version.
> Code changed to check SCO/eSCO type before setting retransmission effort
> and max. latency. The purpose of the patch is to support older devices not
> capable of eSCO.
> 
> Tested on Blackberry 655+ headset which does not support retransmission.
> Credits go to Alexander Sommerhuber.
> 
> Signed-off-by: Bernhard Thaler <bernhard.thaler@r-it.at>
> ---
> net/bluetooth/hci_conn.c |   22 +++++++++++++++++-----
> 1 file changed, 17 insertions(+), 5 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel