Bluetooth: L2CAP updates for PSM validation and ERTM.

PATCH Superseded Mat Martineau 2 participants 19 messages Aug 4, 2010 – Aug 5, 2010

Subsystems (3)

Arm Qualcomm mailing list Bluetooth drivers Bluetooth

lore

Share & tools

lore

Mat Martineau

author

2010-08-04 22:48 UTC #0 lore raw

v2 cover Bluetooth: L2CAP updates for PSM validation and ERTM.

This is a collection of ERTM-related patches, including some L2CAP configuration fixes and allowing partial-frame reads from L2CAP SOCK_STREAM sockets. PSM validation is also included, although that is not restricted to ERTM.

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Mat Martineau

author

2010-08-04 22:48 UTC #1 lore raw

PATCH v2 1/9 Bluetooth: Only enable for L2CAP FCS for ERTM or streaming.

This fixes a bug which caused the FCS setting to show L2CAP_FCS_CRC16 with L2CAP modes other than ERTM or streaming. At present, this only affects the FCS value shown with getsockopt() for basic mode.

Signed-off-by Mat Martineau <[email protected]>

---
 net/bluetooth/l2cap.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

net/bluetooth/l2cap.c +13 -4

index 9ba1e8e..a2706d9 100644
@@ -3063,6 +3063,17 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	return 0;
 }
 
+static inline int l2cap_fcs_needed(struct l2cap_pinfo *pi)
+{
+	if (pi->mode != L2CAP_MODE_ERTM && pi->mode != L2CAP_MODE_STREAMING)
+		return 0;
+	else {
+		/* FCS is enabled if one or both sides request it. */
+		return !(pi->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
+			pi->fcs == L2CAP_FCS_CRC16;
+	}
+}
+
 static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr *cmd, u16 cmd_len, u8 *data)
 {
 	struct l2cap_conf_req *req = (struct l2cap_conf_req *) data;

@@ -3127,8 +3138,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 		goto unlock;
 
 	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
-		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
-		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
+		if (l2cap_fcs_needed(l2cap_pi(sk)))
 			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
 
 		sk->sk_state = BT_CONNECTED;

@@ -3217,8 +3227,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	l2cap_pi(sk)->conf_state |= L2CAP_CONF_INPUT_DONE;
 
 	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_OUTPUT_DONE) {
-		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
-		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
+		if (l2cap_fcs_needed(l2cap_pi(sk)))
 			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
 
 		sk->sk_state = BT_CONNECTED;

Signature · 6 lines

1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Gustavo F. Padovan

2010-08-05 03:31 UTC #2 lore raw

Re: [PATCH 1/9] Bluetooth: Only enable for L2CAP FCS for ERTM or streaming.

Hi Mat,

Quoted 16 lines · Mat Martineau

This fixes a bug which caused the FCS setting to show L2CAP_FCS_CRC16 with L2CAP modes other than ERTM or streaming. At present, this only affects the FCS value shown with getsockopt() for basic mode.
Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)
net/bluetooth/l2cap.c +0 -0
index 9ba1e8e..a2706d9 100644
@@ -3063,6 +3063,17 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	return 0;
 }

net/bluetooth/l2cap.c static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd · line 3074

Quoted 10 lines

net/bluetooth/l2cap.c +9 -0

 
+static inline int l2cap_fcs_needed(struct l2cap_pinfo *pi)
+{
+	if (pi->mode != L2CAP_MODE_ERTM && pi->mode != L2CAP_MODE_STREAMING)
+		return 0;
+	else {
+		/* FCS is enabled if one or both sides request it. */
+		return !(pi->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
+			pi->fcs == L2CAP_FCS_CRC16;
+	}

Get ride of the else, just put the return !(pi->....

Also I would like to see the use case for the check for the ERTM and Streaming before merge this patch. ;)

Signature · 2 lines

Gustavo F. Padovan
http://padovan.org

Mat Martineau

author

2010-08-05 16:27 UTC #3 lore raw

Re: [PATCH 1/9] Bluetooth: Only enable for L2CAP FCS for ERTM or streaming.

Gustavo -

Quoted 21 lines · Gustavo F. Padovan

Hi Mat,
Quoted 17 lines · Mat Martineau
This fixes a bug which caused the FCS setting to show L2CAP_FCS_CRC16 with L2CAP modes other than ERTM or streaming. At present, this only affects the FCS value shown with getsockopt() for basic mode.
Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)
net/bluetooth/l2cap.c +0 -0
index 9ba1e8e..a2706d9 100644
@@ -3063,6 +3063,17 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	return 0;
 }
 

Quoted 11 lines

net/bluetooth/l2cap.c static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd · line 3074
Quoted 9 lines
net/bluetooth/l2cap.c +9 -0
+static inline int l2cap_fcs_needed(struct l2cap_pinfo *pi)
+{
+	if (pi->mode != L2CAP_MODE_ERTM && pi->mode != L2CAP_MODE_STREAMING)
+		return 0;
+	else {
+		/* FCS is enabled if one or both sides request it. */
+		return !(pi->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
+			pi->fcs == L2CAP_FCS_CRC16;
+	}
Get ride of the else, just put the return !(pi->....

Ok.

Quoted 2 lines

Also I would like to see the use case for the check for the ERTM and Streaming before merge this patch. ;)

I have a patch modifying l2cap_skbuff_fromiovec() that allows ERTM PDUs to be longer than the HCI MTU (some USB BT dongles only have a 310 byte HCI MTU). That function has to allocate space for the FCS in the final HCI continuation fragment, and therefore checks to see if FCS is enabled. When I first did this, it was not obvious that pi->fcs could be L2CAP_FCS_CRC16 even when the FCS is not in use.

I hope you can agree that this patch makes the meaning of pi->fcs more intuitive, and eliminates the need to check both pi->fcs and pi->mode to really determine if the FCS is in use. Depending on using pi->fcs only in ERTM code paths makes the code more fragile.

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Gustavo F. Padovan

2010-08-05 03:38 UTC #4 lore raw

Re: [PATCH 1/9] Bluetooth: Only enable for L2CAP FCS for ERTM or streaming.

In reply to Mat Martineau

HI Mat,

You are adding to spaces after "Bluetooth:" and the commit message, one is fine. All the patches have this issue.

Quoted 9 lines · Mat Martineau

This fixes a bug which caused the FCS setting to show L2CAP_FCS_CRC16 with L2CAP modes other than ERTM or streaming. At present, this only affects the FCS value shown with getsockopt() for basic mode.
Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

Signature · 2 lines

Gustavo F. Padovan
http://padovan.org

Mat Martineau

author

2010-08-04 22:48 UTC #5 lore raw

PATCH v2 2/9 Bluetooth: Change default ERTM retransmit timeout.

In reply to Mat Martineau

The L2CAP spec requires that the ERTM retransmit timeout be at least 2 seconds for BR/EDR connections.

Signed-off-by Mat Martineau <[email protected]>

---
 include/net/bluetooth/l2cap.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

include/net/bluetooth/l2cap.h +1 -1

index 636724b..16e412f 100644
@@ -33,7 +33,7 @@
 #define L2CAP_DEFAULT_FLUSH_TO		0xffff
 #define L2CAP_DEFAULT_TX_WINDOW		63
 #define L2CAP_DEFAULT_MAX_TX		3
-#define L2CAP_DEFAULT_RETRANS_TO	1000    /* 1 second */
+#define L2CAP_DEFAULT_RETRANS_TO	2000    /* 2 seconds */
 #define L2CAP_DEFAULT_MONITOR_TO	12000   /* 12 seconds */
 #define L2CAP_DEFAULT_MAX_PDU_SIZE	672
 #define L2CAP_DEFAULT_ACK_TO		200

Signature · 6 lines

1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Gustavo F. Padovan

2010-08-05 03:28 UTC #6 lore raw

Re: [PATCH 2/9] Bluetooth: Change default ERTM retransmit timeout.

Hi Mat,

Quoted 13 lines · Mat Martineau

The L2CAP spec requires that the ERTM retransmit timeout be at least 2 seconds for BR/EDR connections.
Signed-off-by Mat Martineau <[email protected]>
---
 include/net/bluetooth/l2cap.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
include/net/bluetooth/l2cap.h +0 -0
index 636724b..16e412f 100644

include/net/bluetooth/l2cap.h line 39

Quoted 8 lines

include/net/bluetooth/l2cap.h +1 -1

@@ -33,7 +33,7 @@
 #define L2CAP_DEFAULT_FLUSH_TO		0xffff
 #define L2CAP_DEFAULT_TX_WINDOW		63
 #define L2CAP_DEFAULT_MAX_TX		3
-#define L2CAP_DEFAULT_RETRANS_TO	1000    /* 1 second */
+#define L2CAP_DEFAULT_RETRANS_TO	2000    /* 2 seconds */
 #define L2CAP_DEFAULT_MONITOR_TO	12000   /* 12 seconds */
 #define L2CAP_DEFAULT_MAX_PDU_SIZE	672
 #define L2CAP_DEFAULT_ACK_TO		200

The spec says that a 2 seconds retransmission timeout shall be used after a move channel operation in a BR/EDR radio. (section 8.6.2.3) For a normal ACL connection the default value is 1 second(section 8.6.2.1), so I prefer to keep L2CAP_DEFAULT_RETRANS_TO set to 1000.

Signature · 2 lines

Gustavo F. Padovan
http://padovan.org

Mat Martineau

author

2010-08-05 15:53 UTC #7 lore raw

Re: [PATCH 2/9] Bluetooth: Change default ERTM retransmit timeout.

Hi Gustavo -

Quoted 22 lines · Gustavo F. Padovan

Hi Mat,
Quoted 18 lines · Mat Martineau
The L2CAP spec requires that the ERTM retransmit timeout be at least 2 seconds for BR/EDR connections.
Signed-off-by Mat Martineau <[email protected]>
---
 include/net/bluetooth/l2cap.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
include/net/bluetooth/l2cap.h +1 -1
index 636724b..16e412f 100644
@@ -33,7 +33,7 @@
 #define L2CAP_DEFAULT_FLUSH_TO		0xffff
 #define L2CAP_DEFAULT_TX_WINDOW		63
 #define L2CAP_DEFAULT_MAX_TX		3
-#define L2CAP_DEFAULT_RETRANS_TO	1000    /* 1 second */
+#define L2CAP_DEFAULT_RETRANS_TO	2000    /* 2 seconds */

Quoted 8 lines

include/net/bluetooth/l2cap.h line 39
Quoted 3 lines
include/net/bluetooth/l2cap.h +0 -0
 #define L2CAP_DEFAULT_MONITOR_TO	12000   /* 12 seconds */
 #define L2CAP_DEFAULT_MAX_PDU_SIZE	672
 #define L2CAP_DEFAULT_ACK_TO		200
The spec says that a 2 seconds retransmission timeout shall be used after a move channel operation in a BR/EDR radio. (section 8.6.2.3) For a normal ACL connection the default value is 1 second(section 8.6.2.1), so I prefer to keep L2CAP_DEFAULT_RETRANS_TO set to 1000.

Section 8.6.2.1 reads:

"If a flush timeout does not exist on the BR/EDR link for the channel using Enhanced Retransmission mode, then the value for the Retransmission timeout shall be at least two seconds..."

That is followed by the rule for channels with a flush timeout, which requires the retrans timeout to be the larger of flushTO * 3 or 1 second. Since BlueZ does not configure the flush timeout for BR/EDR links, the 2 second rule applies.

I experienced interoperability problems with ERTM on the BM3 Bluetooth stack and the 1-second timeout - the one second timeout was not long enough to allow for all the transmit buffering in BlueZ (all frames to fill the TX window are immediately pushed to the HCI TX queue). I'd like to discuss changing the way ERTM queues data to HCI, but that's a separate topic - maybe something to chat about in Boston.

Regards,

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Mat Martineau

author

2010-08-04 22:49 UTC #8 lore raw

PATCH v2 3/9 Bluetooth: Validate PSM values in calls to connect() and bind().

In reply to Mat Martineau

Valid L2CAP PSMs are odd numbers, and the least significant bit of the most significant byte must be 0.

Signed-off-by Mat Martineau <[email protected]>

---
 net/bluetooth/l2cap.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

net/bluetooth/l2cap.c +12 -0

index a2706d9..920a53f 100644
@@ -1008,6 +1008,12 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
 		goto done;
 	}
 
+	/* If specified, PSM must be odd and lsb of upper byte must be 0 */
+	if (la.l2_psm && (__le16_to_cpu(la.l2_psm) & 0x0101) != 0x0001) {
+		err = -EINVAL;
+		goto done;
+	}
+
 	if (la.l2_psm && __le16_to_cpu(la.l2_psm) < 0x1001 &&
 				!capable(CAP_NET_BIND_SERVICE)) {
 		err = -EACCES;

@@ -1190,6 +1196,12 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr, int al
 		goto done;
 	}
 
+	/* PSM must be odd and lsb of upper byte must be 0 */
+	if ((__le16_to_cpu(la.l2_psm) & 0x0101) != 0x0001) {
+		err = -EINVAL;
+		goto done;
+	}
+
 	/* Set destination address and psm */
 	bacpy(&bt_sk(sk)->dst, &la.l2_bdaddr);
 	l2cap_pi(sk)->psm = la.l2_psm;

Signature · 6 lines

1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Mat Martineau

author

2010-08-04 22:49 UTC #9 lore raw

PATCH v2 4/9 Bluetooth: Fix endianness issue with L2CAP MPS configuration.

In reply to Mat Martineau

Incoming configuration values must be converted to native CPU order before use. This fixes a bug where a little-endian MPS value is compared to a native CPU value. On big-endian processors, this can cause ERTM and streaming mode segmentation to produce PDUs that are larger than the remote stack is expecting, or that would produce fragmented skbs that the current FCS code cannot handle.

Signed-off-by Mat Martineau <[email protected]>

---
 net/bluetooth/l2cap.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

net/bluetooth/l2cap.c +4 -5

index 920a53f..8cf9569 100644
@@ -2708,10 +2708,10 @@ done:
 		case L2CAP_MODE_ERTM:
 			pi->remote_tx_win = rfc.txwin_size;
 			pi->remote_max_tx = rfc.max_transmit;
-			if (rfc.max_pdu_size > pi->conn->mtu - 10)
-				rfc.max_pdu_size = le16_to_cpu(pi->conn->mtu - 10);
 
 			pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);
+			if (pi->remote_mps > pi->conn->mtu - 10)
+				pi->remote_mps = pi->conn->mtu - 10;
 
 			rfc.retrans_timeout =
 				le16_to_cpu(L2CAP_DEFAULT_RETRANS_TO);

@@ -2726,10 +2726,9 @@ done:
 			break;
 
 		case L2CAP_MODE_STREAMING:
-			if (rfc.max_pdu_size > pi->conn->mtu - 10)
-				rfc.max_pdu_size = le16_to_cpu(pi->conn->mtu - 10);
-
 			pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);
+			if (pi->remote_mps > pi->conn->mtu - 10)
+				pi->remote_mps = pi->conn->mtu - 10;
 
 			pi->conf_state |= L2CAP_CONF_MODE_DONE;

Signature · 6 lines

1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Gustavo F. Padovan

2010-08-05 04:00 UTC #10 lore raw

Re: [PATCH 4/9] Bluetooth: Fix endianness issue with L2CAP MPS configuration.

Hi Mat,

Quoted 17 lines · Mat Martineau

Incoming configuration values must be converted to native CPU order before use. This fixes a bug where a little-endian MPS value is compared to a native CPU value. On big-endian processors, this can cause ERTM and streaming mode segmentation to produce PDUs that are larger than the remote stack is expecting, or that would produce fragmented skbs that the current FCS code cannot handle.
Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)
net/bluetooth/l2cap.c +0 -0
index 920a53f..8cf9569 100644

net/bluetooth/l2cap.c done: · line 2714

Quoted 9 lines

net/bluetooth/l2cap.c +2 -2

@@ -2708,10 +2708,10 @@ done:
 		case L2CAP_MODE_ERTM:
 			pi->remote_tx_win = rfc.txwin_size;
 			pi->remote_max_tx = rfc.max_transmit;
-			if (rfc.max_pdu_size > pi->conn->mtu - 10)
-				rfc.max_pdu_size = le16_to_cpu(pi->conn->mtu - 10);
 
 			pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);
+			if (pi->remote_mps > pi->conn->mtu - 10)
+				pi->remote_mps = pi->conn->mtu - 10;

What happened with thte "rfc.max_pdu_size =" attribution. We have the send the value to through the RFC option. So what I do propose here is:

net/bluetooth/l2cap.c +2 -2

index 0f34e12..11d4405 100644
@@ -2705,7 +2705,7 @@ done:
                case L2CAP_MODE_ERTM:
                        pi->remote_tx_win = rfc.txwin_size;
                        pi->remote_max_tx = rfc.max_transmit;
-                       if (rfc.max_pdu_size > pi->conn->mtu - 10)
+                       if (le16_to_cpu(rfc.max_pdu_size) > pi->conn->mtu - 10)
                                rfc.max_pdu_size = le16_to_cpu(pi->conn->mtu - 10);
 
                        pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);

@@ -2723,7 +2723,7 @@ done:
                        break;
 
                case L2CAP_MODE_STREAMING:
-                       if (rfc.max_pdu_size > pi->conn->mtu - 10)
+                       if (le16_to_cpu(rfc.max_pdu_size) > pi->conn->mtu - 10)
                                rfc.max_pdu_size = le16_to_cpu(pi->conn->mtu - 10);
 
                        pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);

Quoted 15 lines

net/bluetooth/l2cap.c +2 -3

 			rfc.retrans_timeout =
 				le16_to_cpu(L2CAP_DEFAULT_RETRANS_TO);

@@ -2726,10 +2726,9 @@ done:
 			break;
 
 		case L2CAP_MODE_STREAMING:
-			if (rfc.max_pdu_size > pi->conn->mtu - 10)
-				rfc.max_pdu_size = le16_to_cpu(pi->conn->mtu - 10);
-
 			pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);
+			if (pi->remote_mps > pi->conn->mtu - 10)
+				pi->remote_mps = pi->conn->mtu - 10;
 
 			pi->conf_state |= L2CAP_CONF_MODE_DONE;

Signature · 2 lines

Gustavo F. Padovan
http://padovan.org

Mat Martineau

author

2010-08-05 16:50 UTC #11 lore raw

Re: [PATCH 4/9] Bluetooth: Fix endianness issue with L2CAP MPS configuration.

Gustavo -

Quoted 27 lines · Gustavo F. Padovan

Hi Mat,
Quoted 23 lines · Mat Martineau
Incoming configuration values must be converted to native CPU order before use. This fixes a bug where a little-endian MPS value is compared to a native CPU value. On big-endian processors, this can cause ERTM and streaming mode segmentation to produce PDUs that are larger than the remote stack is expecting, or that would produce fragmented skbs that the current FCS code cannot handle.
Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)
net/bluetooth/l2cap.c +0 -2
index 920a53f..8cf9569 100644
@@ -2708,10 +2708,10 @@ done:
 		case L2CAP_MODE_ERTM:
 			pi->remote_tx_win = rfc.txwin_size;
 			pi->remote_max_tx = rfc.max_transmit;
-			if (rfc.max_pdu_size > pi->conn->mtu - 10)
-				rfc.max_pdu_size = le16_to_cpu(pi->conn->mtu - 10);
 

Quoted 7 lines

net/bluetooth/l2cap.c done: · line 2714
Quoted 3 lines
net/bluetooth/l2cap.c +2 -0
 			pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);
+			if (pi->remote_mps > pi->conn->mtu - 10)
+				pi->remote_mps = pi->conn->mtu - 10;
What happened with thte "rfc.max_pdu_size =" attribution. We have the send the value to through the RFC option.

You're right - I didn't see that the rfc struct was used for both the response and reply.

Quoted 7 lines

So what I do propose here is:
net/bluetooth/l2cap.c +0 -0
index 0f34e12..11d4405 100644

net/bluetooth/l2cap.c done: · line 2711

Quoted 8 lines

net/bluetooth/l2cap.c +1 -1

@@ -2705,7 +2705,7 @@ done:
               case L2CAP_MODE_ERTM:
                       pi->remote_tx_win = rfc.txwin_size;
                       pi->remote_max_tx = rfc.max_transmit;
-                       if (rfc.max_pdu_size > pi->conn->mtu - 10)
+                       if (le16_to_cpu(rfc.max_pdu_size) > pi->conn->mtu - 10)
                               rfc.max_pdu_size = le16_to_cpu(pi->conn->mtu - 10);
 
                       pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);

Sure. I'll change it to "cpu_to_le16(pi->conn->mtu - 10)" (even though they're functionally equivalent, it is definitely a conversion to le16).

net/bluetooth/l2cap.c done: · line 2727

Quoted 7 lines

net/bluetooth/l2cap.c +1 -1

@@ -2723,7 +2723,7 @@ done:
                       break;
 
               case L2CAP_MODE_STREAMING:
-                       if (rfc.max_pdu_size > pi->conn->mtu - 10)
+                       if (le16_to_cpu(rfc.max_pdu_size) > pi->conn->mtu - 10)
                               rfc.max_pdu_size = le16_to_cpu(pi->conn->mtu - 10);

Same thing - cpu_to_le16() instead.

Quoted 18 lines

net/bluetooth/l2cap.c +0 -0

                       pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);

Quoted 16 lines

net/bluetooth/l2cap.c +2 -3

 			rfc.retrans_timeout =
 				le16_to_cpu(L2CAP_DEFAULT_RETRANS_TO);

@@ -2726,10 +2726,9 @@ done:
 			break;
 
 		case L2CAP_MODE_STREAMING:
-			if (rfc.max_pdu_size > pi->conn->mtu - 10)
-				rfc.max_pdu_size = le16_to_cpu(pi->conn->mtu - 10);
-
 			pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);
+			if (pi->remote_mps > pi->conn->mtu - 10)
+				pi->remote_mps = pi->conn->mtu - 10;

Quoted 1 line

net/bluetooth/l2cap.c done: · line 2733
Quoted 1 line
net/bluetooth/l2cap.c +0 -0
 			pi->conf_state |= L2CAP_CONF_MODE_DONE;

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Mat Martineau

author

2010-08-04 22:49 UTC #12 lore raw

PATCH v2 5/9 Bluetooth: Fix incorrect setting of remote_tx_win for L2CAP ERTM.

In reply to Mat Martineau

remote_tx_win is intended to be set on receipt of an L2CAP configuration request. The value is used to determine the size of the transmit window on the remote side of an ERTM connection, so L2CAP can stop sending frames when that remote window is full.

An incorrect remote_tx_win value will cause the stack to not fully utilize the tx window (performance impact), or to overfill the remote tx window (causing dropped frames or a disconnect).

This patch removes an extra setting of remote_tx_win when a configuration response is received. The transmit window has a different meaning in a response - it is an informational value less than or equal to the local tx_win.

Signed-off-by Mat Martineau <[email protected]>

---
 net/bluetooth/l2cap.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

net/bluetooth/l2cap.c +0 -1

index 8cf9569..f0f3c7c 100644
@@ -2808,7 +2808,6 @@ static int l2cap_parse_conf_rsp(struct sock *sk, void *rsp, int len, void *data,
 	if (*result == L2CAP_CONF_SUCCESS) {
 		switch (rfc.mode) {
 		case L2CAP_MODE_ERTM:
-			pi->remote_tx_win   = rfc.txwin_size;
 			pi->retrans_timeout = le16_to_cpu(rfc.retrans_timeout);
 			pi->monitor_timeout = le16_to_cpu(rfc.monitor_timeout);
 			pi->mps    = le16_to_cpu(rfc.max_pdu_size);

Signature · 6 lines

1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Gustavo F. Padovan

2010-08-05 04:19 UTC #13 lore raw

Re: [PATCH 5/9] Bluetooth: Fix incorrect setting of remote_tx_win for L2CAP ERTM.

Hi Mat,

Quoted 24 lines · Mat Martineau

remote_tx_win is intended to be set on receipt of an L2CAP configuration request. The value is used to determine the size of the transmit window on the remote side of an ERTM connection, so L2CAP can stop sending frames when that remote window is full.
An incorrect remote_tx_win value will cause the stack to not fully utilize the tx window (performance impact), or to overfill the remote tx window (causing dropped frames or a disconnect).
This patch removes an extra setting of remote_tx_win when a configuration response is received. The transmit window has a different meaning in a response - it is an informational value less than or equal to the local tx_win.
Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
net/bluetooth/l2cap.c +0 -0
index 8cf9569..f0f3c7c 100644

net/bluetooth/l2cap.c static int l2cap_parse_conf_rsp(struct sock *sk, void *rsp, int len, void *data, · line 2813

Quoted 7 lines

net/bluetooth/l2cap.c +0 -1

@@ -2808,7 +2808,6 @@ static int l2cap_parse_conf_rsp(struct sock *sk, void *rsp, int len, void *data,
 	if (*result == L2CAP_CONF_SUCCESS) {
 		switch (rfc.mode) {
 		case L2CAP_MODE_ERTM:
-			pi->remote_tx_win   = rfc.txwin_size;
 			pi->retrans_timeout = le16_to_cpu(rfc.retrans_timeout);
 			pi->monitor_timeout = le16_to_cpu(rfc.monitor_timeout);
 			pi->mps    = le16_to_cpu(rfc.max_pdu_size);

I agree. But you may also want to remove the same check inside l2cap_conf_rfc_get()

Signature · 2 lines

Gustavo F. Padovan
http://padovan.org

Mat Martineau

author

2010-08-05 16:54 UTC #14 lore raw

Re: [PATCH 5/9] Bluetooth: Fix incorrect setting of remote_tx_win for L2CAP ERTM.

Gustavo -

Quoted 23 lines · Gustavo F. Padovan

Hi Mat,
Quoted 19 lines · Mat Martineau
remote_tx_win is intended to be set on receipt of an L2CAP configuration request. The value is used to determine the size of the transmit window on the remote side of an ERTM connection, so L2CAP can stop sending frames when that remote window is full.
An incorrect remote_tx_win value will cause the stack to not fully utilize the tx window (performance impact), or to overfill the remote tx window (causing dropped frames or a disconnect).
This patch removes an extra setting of remote_tx_win when a configuration response is received. The transmit window has a different meaning in a response - it is an informational value less than or equal to the local tx_win.
Signed-off-by Mat Martineau <[email protected]>
---
 net/bluetooth/l2cap.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

Quoted 15 lines

net/bluetooth/l2cap.c static int l2cap_parse_conf_rsp(struct sock *sk, void *rsp, int len, void *data, · line 2813
Quoted 12 lines
net/bluetooth/l2cap.c +0 -1
index 8cf9569..f0f3c7c 100644
@@ -2808,7 +2808,6 @@ static int l2cap_parse_conf_rsp(struct sock *sk, void *rsp, int len, void *data,
 	if (*result == L2CAP_CONF_SUCCESS) {
 		switch (rfc.mode) {
 		case L2CAP_MODE_ERTM:
-			pi->remote_tx_win   = rfc.txwin_size;
 			pi->retrans_timeout = le16_to_cpu(rfc.retrans_timeout);
 			pi->monitor_timeout = le16_to_cpu(rfc.monitor_timeout);
 			pi->mps    = le16_to_cpu(rfc.max_pdu_size);
I agree. But you may also want to remove the same check inside l2cap_conf_rfc_get()

I'll do that.

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Mat Martineau

author

2010-08-04 22:49 UTC #15 lore raw

PATCH v2 6/9 Bluetooth: Add common code for stream-oriented recvmsg()

In reply to Mat Martineau

This commit adds a bt_sock_stream_recvmsg() function for use by any Bluetooth code that uses SOCK_STREAM sockets. This code is copied from rfcomm_sock_recvmsg() with minimal modifications to remove RFCOMM-specific functionality and improve readability.

L2CAP (with the SOCK_STREAM socket type) and RFCOMM have common needs when it comes to reading data. Proper stream read semantics require that applications can read from a stream one byte at a time and not lose any data. The RFCOMM code already operated on and pulled data from the underlying L2CAP socket, so very few changes were required to make the code more generic for use with non-RFCOMM data over L2CAP.

Applications that need more awareness of L2CAP frame boundaries are still free to use SOCK_SEQPACKET sockets, and may verify that they connection did not fall back to basic mode by calling getsockopt().

Signed-off-by Mat Martineau <[email protected]>

---
 include/net/bluetooth/bluetooth.h |    2 +
 net/bluetooth/af_bluetooth.c      |  109 +++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 0 deletions(-)

include/net/bluetooth/bluetooth.h +2 -0

index 27a902d..08b6c2a 100644
@@ -126,6 +126,8 @@ int  bt_sock_unregister(int proto);
 void bt_sock_link(struct bt_sock_list *l, struct sock *s);
 void bt_sock_unlink(struct bt_sock_list *l, struct sock *s);
 int  bt_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct msghdr *msg, size_t len, int flags);
+int  bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
+			struct msghdr *msg, size_t len, int flags);
 uint bt_sock_poll(struct file * file, struct socket *sock, poll_table *wait);
 int  bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
 int  bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);

net/bluetooth/af_bluetooth.c +109 -0

115 lines not on this page · raw message

Signature · 6 lines

1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Mat Martineau

author

2010-08-04 22:49 UTC #16 lore raw

PATCH v2 7/9 Bluetooth: Use common SOCK_STREAM receive code in RFCOMM.

In reply to Mat Martineau

To reduce code duplication, have rfcomm_sock_recvmsg() call bt_sock_stream_recvmsg(). The common bt_sock_stream_recvmsg() code is nearly identical, with the RFCOMM-specific functionality for deferred setup and connection unthrottling left in rfcomm_sock_recvmsg().

Signed-off-by Mat Martineau <[email protected]>

---
 net/bluetooth/rfcomm/sock.c |  104 +++----------------------------------------
 1 files changed, 6 insertions(+), 98 deletions(-)

net/bluetooth/rfcomm/sock.c +6 ~~-98~~

index 44a6232..4396f47 100644
@@ -617,121 +617,29 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
 	return sent;
 }
 
-static long rfcomm_sock_data_wait(struct sock *sk, long timeo)
-{
-	DECLARE_WAITQUEUE(wait, current);
-
-	add_wait_queue(sk_sleep(sk), &wait);
-	for (;;) {
-		set_current_state(TASK_INTERRUPTIBLE);
-
-		if (!skb_queue_empty(&sk->sk_receive_queue) ||
-		    sk->sk_err ||
-		    (sk->sk_shutdown & RCV_SHUTDOWN) ||
-		    signal_pending(current) ||
-		    !timeo)
-			break;
-
-		set_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
-		release_sock(sk);
-		timeo = schedule_timeout(timeo);
-		lock_sock(sk);
-		clear_bit(SOCK_ASYNC_WAITDATA, &sk->sk_socket->flags);
-	}
-
-	__set_current_state(TASK_RUNNING);
-	remove_wait_queue(sk_sleep(sk), &wait);
-	return timeo;
-}
-
 static int rfcomm_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
 			       struct msghdr *msg, size_t size, int flags)
 {
 	struct sock *sk = sock->sk;
 	struct rfcomm_dlc *d = rfcomm_pi(sk)->dlc;
-	int err = 0;
-	size_t target, copied = 0;
-	long timeo;
+	int len;
 
 	if (test_and_clear_bit(RFCOMM_DEFER_SETUP, &d->flags)) {
 		rfcomm_dlc_accept(d);
 		return 0;
 	}
 
-	if (flags & MSG_OOB)
-		return -EOPNOTSUPP;
-
-	msg->msg_namelen = 0;
-
-	BT_DBG("sk %p size %zu", sk, size);
+	len = bt_sock_stream_recvmsg(iocb, sock, msg, size, flags);
 
 	lock_sock(sk);
+	if (!(flags & MSG_PEEK) && len > 0)
+		atomic_sub(len, &sk->sk_rmem_alloc);
 
-	target = sock_rcvlowat(sk, flags & MSG_WAITALL, size);
-	timeo  = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
-
-	do {
-		struct sk_buff *skb;
-		int chunk;
-
-		skb = skb_dequeue(&sk->sk_receive_queue);
-		if (!skb) {
-			if (copied >= target)
-				break;
-
-			if ((err = sock_error(sk)) != 0)
-				break;
-			if (sk->sk_shutdown & RCV_SHUTDOWN)
-				break;
-
-			err = -EAGAIN;
-			if (!timeo)
-				break;
-
-			timeo = rfcomm_sock_data_wait(sk, timeo);
-
-			if (signal_pending(current)) {
-				err = sock_intr_errno(timeo);
-				goto out;
-			}
-			continue;
-		}
-
-		chunk = min_t(unsigned int, skb->len, size);
-		if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) {
-			skb_queue_head(&sk->sk_receive_queue, skb);
-			if (!copied)
-				copied = -EFAULT;
-			break;
-		}
-		copied += chunk;
-		size   -= chunk;
-
-		sock_recv_ts_and_drops(msg, sk, skb);
-
-		if (!(flags & MSG_PEEK)) {
-			atomic_sub(chunk, &sk->sk_rmem_alloc);
-
-			skb_pull(skb, chunk);
-			if (skb->len) {
-				skb_queue_head(&sk->sk_receive_queue, skb);
-				break;
-			}
-			kfree_skb(skb);
-
-		} else {
-			/* put message back and return */
-			skb_queue_head(&sk->sk_receive_queue, skb);
-			break;
-		}
-	} while (size);
-
-out:
 	if (atomic_read(&sk->sk_rmem_alloc) <= (sk->sk_rcvbuf >> 2))
 		rfcomm_dlc_unthrottle(rfcomm_pi(sk)->dlc);
-
 	release_sock(sk);
-	return copied ? : err;
+
+	return len;
 }
 
 static int rfcomm_sock_setsockopt_old(struct socket *sock, int optname, char __user *optval, unsigned int optlen)

Signature · 6 lines

1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Mat Martineau

author

2010-08-04 22:49 UTC #17 lore raw

PATCH v2 8/9 Bluetooth: Use a stream-oriented recvmsg with SOCK_STREAM L2CAP sockets.

In reply to Mat Martineau

L2CAP ERTM sockets can be opened with the SOCK_STREAM socket type, which is a mandatory request for ERTM mode.

However, these sockets still have SOCK_SEQPACKET read semantics when bt_sock_recvmsg() is used to pull data from the receive queue. If the application is only reading part of a frame, then the unread portion of the frame is discarded. If the application requests more bytes than are in the current frame, only the current frame's data is returned.

This patch utilizes common code derived from RFCOMM's recvmsg() function to make L2CAP SOCK_STREAM reads behave like RFCOMM reads (and other SOCK_STREAM sockets in general). The application may read one byte at a time from the input stream and not lose any data, and may also read across L2CAP frame boundaries.

Signed-off-by Mat Martineau <[email protected]>

---
 net/bluetooth/l2cap.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

net/bluetooth/l2cap.c +4 -1

index f0f3c7c..b9de88d 100644
@@ -1956,7 +1956,10 @@ static int l2cap_sock_recvmsg(struct kiocb *iocb, struct socket *sock, struct ms
 
 	release_sock(sk);
 
-	return bt_sock_recvmsg(iocb, sock, msg, len, flags);
+	if (sock->type == SOCK_STREAM)
+		return bt_sock_stream_recvmsg(iocb, sock, msg, len, flags);
+	else
+		return bt_sock_recvmsg(iocb, sock, msg, len, flags);
 }
 
 static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __user *optval, unsigned int optlen)

Signature · 6 lines

1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Mat Martineau

author

2010-08-04 22:49 UTC #18 lore raw

PATCH v2 9/9 Bluetooth: Use 3-DH5 payload size for default ERTM max PDU size.

In reply to Mat Martineau

The previous value of 672 for L2CAP_DEFAULT_MAX_PDU_SIZE is based on the default L2CAP MTU. That default MTU is calculated from the size of two DH5 packets, minus ACL and L2CAP b-frame header overhead.

ERTM is used with newer basebands that typically support larger 3-DH5 packets, and i-frames and s-frames have more header overhead. With clean RF conditions, basebands will typically attempt to use 1021-byte 3-DH5 packets for maximum throughput. Adjusting for 2 bytes of ACL headers plus 10 bytes of worst-case L2CAP headers yields 1009 bytes of payload.

This PDU size imposes less overhead for header bytes and gives the baseband the option to choose 3-DH5 packets, but is small enough for ERTM traffic to interleave well with other L2CAP or SCO data. 672-byte payloads do not allow the most efficient over-the-air packet choice, and cannot achieve maximum throughput over BR/EDR.

Signed-off-by Mat Martineau <[email protected]>

---
 include/net/bluetooth/l2cap.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

include/net/bluetooth/l2cap.h +1 -1

index 16e412f..6c24144 100644
@@ -35,7 +35,7 @@
 #define L2CAP_DEFAULT_MAX_TX		3
 #define L2CAP_DEFAULT_RETRANS_TO	2000    /* 2 seconds */
 #define L2CAP_DEFAULT_MONITOR_TO	12000   /* 12 seconds */
-#define L2CAP_DEFAULT_MAX_PDU_SIZE	672
+#define L2CAP_DEFAULT_MAX_PDU_SIZE	1009    /* Sized for 3-DH5 packet */
 #define L2CAP_DEFAULT_ACK_TO		200
 #define L2CAP_LOCAL_BUSY_TRIES		12

Signature · 6 lines

1.7.1

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum