[PATCH v2 0/5] Some ERTM improvements

[PATCH v2 0/5] Some ERTM improvements

[Szymon Janc]

No code changes since v1.
Only improved commits messages as requested by Marcel.

Szymon Janc (5):
  Bluetooth: Make l2cap_clear_timer return if timer was running or not
  Bluetooth: Set P-bit for SREJ frame only if there are I-frames to ack
  Bluetooth: Clear ack_timer when sending ack
  Bluetooth: Don't send RNR immediately when entering local busy
  Bluetooth: Drop L2CAP chan reference if ERTM ack_timer fired

 include/net/bluetooth/l2cap.h |    9 +++++++--
 net/bluetooth/l2cap_core.c    |   33 ++++++++++++++++++---------------
 2 files changed, 25 insertions(+), 17 deletions(-)

-- 
on behalf of ST-Ericsson

[PATCH v2 1/5] Bluetooth: Make l2cap_clear_timer return if timer was running or not

[Szymon Janc]

This is usefull when need to make action after timer was cleared
depending on if it was running or not.

Signed-off-by: Szymon Janc <szymon.janc@tieto.com>
---
 include/net/bluetooth/l2cap.h |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index b564142..1ead4c4 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -616,11 +616,16 @@ static inline void l2cap_set_timer(struct l2cap_chan *chan,
 	schedule_delayed_work(work, timeout);
 }
 
-static inline void l2cap_clear_timer(struct l2cap_chan *chan,
+static inline bool l2cap_clear_timer(struct l2cap_chan *chan,
 					struct delayed_work *work)
 {
-	if (__cancel_delayed_work(work))
+	bool ret;
+
+	ret = __cancel_delayed_work(work);
+	if (ret)
 		l2cap_chan_put(chan);
+
+	return ret;
 }
 
 #define __set_chan_timer(c, t) l2cap_set_timer(c, &c->chan_timer, (t))
-- 
on behalf of ST-Ericsson

[PATCH v2 2/5] Bluetooth: Set P-bit for SREJ frame only if there are I-frames to ack

[Szymon Janc]

SREJ frame with P-bit set acknowledges I-frames numbered up to
(ReqSeq - 1). With this patch P-bit in SREJ is set only when there are
some I-frames to ack.

This fixes ambiguous situation when lost of I-frame with TxSeq=0 would
result in sending SREJ acking all previous I-frames.
Consider following scenario:
TxWindow=3

HostA: sent I-frame TxSeq=0
HostA: sent I-frame TxSeq=1
HostA: sent I-frame TxSeq=2
HostB: missed I-frame TxSeq=0
HostB: received I-frame TxSeq=1
HostB: sent SREJ ReqSeq=0 Pbit=1
HostA: received SREJ ReqSeq=0 Pbit=1   <- All I-frames acked or not?
...

Signed-off-by: Szymon Janc <szymon.janc@tieto.com>
---
 net/bluetooth/l2cap_core.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 97f8549..308d9d7 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3927,15 +3927,15 @@ static inline int l2cap_data_channel_iframe(struct l2cap_chan *chan, u32 rx_cont
 		__skb_queue_head_init(&chan->srej_q);
 		l2cap_add_to_srej_queue(chan, skb, tx_seq, sar);
 
-		set_bit(CONN_SEND_PBIT, &chan->conn_state);
+		/* Set P-bit only if there are some I-frames to ack. */
+		if (__clear_ack_timer(chan))
+			set_bit(CONN_SEND_PBIT, &chan->conn_state);
 
 		err = l2cap_send_srejframe(chan, tx_seq);
 		if (err < 0) {
 			l2cap_send_disconn_req(chan->conn, chan, -err);
 			return err;
 		}
-
-		__clear_ack_timer(chan);
 	}
 	return 0;
 
-- 
on behalf of ST-Ericsson

[PATCH v2 3/5] Bluetooth: Clear ack_timer when sending ack

[Szymon Janc]

ack_timer should be cleared when sending ACK to avoid acking I-frames
twice.

This commit introduces helper function (only send ack, not clearing
timer) which is used by l2cap_send_ack and l2cap_ack_timeout. This is
to avoid clearing ack timer in timer function.

Signed-off-by: Szymon Janc <szymon.janc@tieto.com>
---
 net/bluetooth/l2cap_core.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 308d9d7..b9e232e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1478,7 +1478,7 @@ static int l2cap_retransmit_frames(struct l2cap_chan *chan)
 	return ret;
 }
 
-static void l2cap_send_ack(struct l2cap_chan *chan)
+static void __l2cap_send_ack(struct l2cap_chan *chan)
 {
 	u32 control = 0;
 
@@ -1498,6 +1498,12 @@ static void l2cap_send_ack(struct l2cap_chan *chan)
 	l2cap_send_sframe(chan, control);
 }
 
+static void l2cap_send_ack(struct l2cap_chan *chan)
+{
+	__clear_ack_timer(chan);
+	__l2cap_send_ack(chan);
+}
+
 static void l2cap_send_srejtail(struct l2cap_chan *chan)
 {
 	struct srej_list *tail;
@@ -1988,7 +1994,7 @@ static void l2cap_ack_timeout(struct work_struct *work)
 	BT_DBG("chan %p", chan);
 
 	lock_sock(chan->sk);
-	l2cap_send_ack(chan);
+	__l2cap_send_ack(chan);
 	release_sock(chan->sk);
 }
 
-- 
on behalf of ST-Ericsson

[PATCH v2 4/5] Bluetooth: Don't send RNR immediately when entering local busy

[Szymon Janc]

There is no need to send RNR immediately when entring local busy.
Also upper layer might clear local busy condition before ack timer
expires saving few cycles for sending RNR.

This also prevents sending two RNRs in some cases where sending one
would be enough i.e received N I-frame can trigger local busy
(sending RNR acking up to N-1 I-frame) and later sending ack (RNR
acking up to N I-frame).

This was affecting TC_ERM_BV_07_C and TC_ERM_BV_22_C with some non
default channel parameters (tx window and receiving buffer sizes).

Signed-off-by: Szymon Janc <szymon.janc@tieto.com>
---
 net/bluetooth/l2cap_core.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b9e232e..cd1eb6d 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3719,19 +3719,11 @@ static int l2cap_reassemble_sdu(struct l2cap_chan *chan, struct sk_buff *skb, u3
 
 static void l2cap_ertm_enter_local_busy(struct l2cap_chan *chan)
 {
-	u32 control;
-
 	BT_DBG("chan %p, Enter local busy", chan);
 
 	set_bit(CONN_LOCAL_BUSY, &chan->conn_state);
 
-	control = __set_reqseq(chan, chan->buffer_seq);
-	control |= __set_ctrl_super(chan, L2CAP_SUPER_RNR);
-	l2cap_send_sframe(chan, control);
-
-	set_bit(CONN_RNR_SENT, &chan->conn_state);
-
-	__clear_ack_timer(chan);
+	__set_ack_timer(chan);
 }
 
 static void l2cap_ertm_exit_local_busy(struct l2cap_chan *chan)
@@ -3871,8 +3863,11 @@ static inline int l2cap_data_channel_iframe(struct l2cap_chan *chan, u32 rx_cont
 		goto drop;
 	}
 
-	if (test_bit(CONN_LOCAL_BUSY, &chan->conn_state))
+	if (test_bit(CONN_LOCAL_BUSY, &chan->conn_state)) {
+		if (!test_bit(CONN_RNR_SENT, &chan->conn_state))
+			l2cap_send_ack(chan);
 		goto drop;
+	}
 
 	if (tx_seq == chan->expected_tx_seq)
 		goto expected;
-- 
on behalf of ST-Ericsson

[PATCH v2 5/5] Bluetooth: Drop L2CAP chan reference if ERTM ack_timer fired

[Szymon Janc]

Reference counter was incremented when starting ack timer but
decremented only when clearing timer, not when timer fired.

Signed-off-by: Szymon Janc <szymon.janc@tieto.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/l2cap_core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index cd1eb6d..f4d2e4be 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1996,6 +1996,8 @@ static void l2cap_ack_timeout(struct work_struct *work)
 	lock_sock(chan->sk);
 	__l2cap_send_ack(chan);
 	release_sock(chan->sk);
+
+	l2cap_chan_put(chan);
 }
 
 static inline void l2cap_ertm_init(struct l2cap_chan *chan)
-- 
on behalf of ST-Ericsson

Re: [PATCH v2 0/5] Some ERTM improvements

[Marcel Holtmann]

Hi Szymon,

> No code changes since v1.
> Only improved commits messages as requested by Marcel.
> 
> Szymon Janc (5):
>   Bluetooth: Make l2cap_clear_timer return if timer was running or not
>   Bluetooth: Set P-bit for SREJ frame only if there are I-frames to ack
>   Bluetooth: Clear ack_timer when sending ack
>   Bluetooth: Don't send RNR immediately when entering local busy
>   Bluetooth: Drop L2CAP chan reference if ERTM ack_timer fired
> 
>  include/net/bluetooth/l2cap.h |    9 +++++++--
>  net/bluetooth/l2cap_core.c    |   33 ++++++++++++++++++---------------
>  2 files changed, 25 insertions(+), 17 deletions(-)

looks all good now. Thanks for updating the commit messages.

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Johan, feel free to take the whole series.

Regards

Marcel

Re: [PATCH v2 0/5] Some ERTM improvements

[Johan Hedberg]

Hi Szymon,

On Wed, Jan 11, 2012, Szymon Janc wrote:
> No code changes since v1.
> Only improved commits messages as requested by Marcel.
> 
> Szymon Janc (5):
>   Bluetooth: Make l2cap_clear_timer return if timer was running or not
>   Bluetooth: Set P-bit for SREJ frame only if there are I-frames to ack
>   Bluetooth: Clear ack_timer when sending ack
>   Bluetooth: Don't send RNR immediately when entering local busy
>   Bluetooth: Drop L2CAP chan reference if ERTM ack_timer fired
> 
>  include/net/bluetooth/l2cap.h |    9 +++++++--
>  net/bluetooth/l2cap_core.c    |   33 ++++++++++++++++++---------------
>  2 files changed, 25 insertions(+), 17 deletions(-)

All five patches have been applied to my bluetooth-next tree. Thanks.

Johan