[PATCH 0/5] ERTM improvements

[PATCH 0/5] ERTM improvements

[Szymon Janc]

Hi,

Those patches introduce some improvements to ERTM related to avoiding
acking I-frames more than once.

Following PTS test-cases were affected:
TC_ERM_BV_07_C
TC_ERM_BV_22_C

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 1/5] Bluetooth: Make l2cap_clear_timer return if timer was running or not

[Szymon Janc]

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

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

[Marcel Holtmann]

Hi Szymon,

> Signed-off-by: Szymon Janc <szymon.janc@tieto.com>

so I am enforcing this now. No more empty commit message bodies!

Explain your patch and why do you need it. This is the perfect example
where you mention ahead of time why you want this.

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

Regards

Marcel

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

[Szymon Janc]

> Hi Szymon,

Hi,

> so I am enforcing this now. No more empty commit message bodies!
> 
> Explain your patch and why do you need it. This is the perfect example
> where you mention ahead of time why you want this.

I'll squash 1 and 2 then and add some more explanation.

BR
Szymon Janc

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

[Marcel Holtmann]

Hi Szymon,

> > so I am enforcing this now. No more empty commit message bodies!
> > 
> > Explain your patch and why do you need it. This is the perfect example
> > where you mention ahead of time why you want this.
> 
> I'll squash 1 and 2 then and add some more explanation.

I prefer you keep the separation. The splitting of your patches is
actually the part I liked a lot. They were logically separated. You just
need to be a bit more verbose with your commit messages to describe what
you are doing and why.

Bad luck that you are the first one where I am enforcing this now, but
that is just life. Blame all the others that send in patches with crappy
commit messages ;)

Regards

Marcel

[PATCH 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). Also stop ack timer before sending SREJ with P-bit set,
otherwise I-frames would be acknowledged twice.

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

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

[Marcel Holtmann]

Hi Szymon,

> SREJ frame with P-bit set acknowledges I-frames numbered up to
> (ReqSeq - 1). Also stop ack timer before sending SREJ with P-bit set,
> otherwise I-frames would be acknowledged twice.

some notes about the qualification test cases and a bit more verbose
human explanation is expected here as well.

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

Regards

Marcel

[PATCH 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.

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

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

[Marcel Holtmann]

Hi Szymon,

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

explain why you are creating a helper for it. Since that is clearly not
obvious to me.

> 
> 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);

I would have just added

	__clear_ack_timer(chan):

here.

> -	l2cap_send_ack(chan);
> +	__l2cap_send_ack(chan);
>  	release_sock(chan->sk);
>  }
>  

Regards

Marcel

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

[Szymon Janc]

> Hi Szymon,

Hi,

> > ack_timer should be cleared when sending ACK to avoid acking I-frames
> > twice.
> 
> explain why you are creating a helper for it. Since that is clearly not
> obvious to me.

To avoid code duplication - helper is not clearing ack timer and is used by
both l2cap_send_ack and l2cap_ack_timeout.
There is no need to clear ack timer in timer funtion.

But if you prefer to not have helper and are fine with clearing ack timer also in
l2cap_ack_timeout then ack timer can be clear in l2cap_send_ack.


> > 
> > 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);
> 
> I would have just added
> 
> 	__clear_ack_timer(chan):
> 
> here.

This is a timer function so there is no need to clear timer here.

> > -	l2cap_send_ack(chan);
> > +	__l2cap_send_ack(chan);
> >  	release_sock(chan->sk);
> >  }
> >  

BR
Szymon Janc

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

[Marcel Holtmann]

Hi Szymon,

> > > ack_timer should be cleared when sending ACK to avoid acking I-frames
> > > twice.
> > 
> > explain why you are creating a helper for it. Since that is clearly not
> > obvious to me.
> 
> To avoid code duplication - helper is not clearing ack timer and is used by
> both l2cap_send_ack and l2cap_ack_timeout.
> There is no need to clear ack timer in timer funtion.
> 
> But if you prefer to not have helper and are fine with clearing ack timer also in
> l2cap_ack_timeout then ack timer can be clear in l2cap_send_ack.

put that in the commit message please.

> > > 
> > > 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);
> > 
> > I would have just added
> > 
> > 	__clear_ack_timer(chan):
> > 
> > here.
> 
> This is a timer function so there is no need to clear timer here.
> 
> > > -	l2cap_send_ack(chan);
> > > +	__l2cap_send_ack(chan);
> > >  	release_sock(chan->sk);
> > >  }
> > >  

You are absolutely correct of course. I confused myself here. This also
means that either we add proper comments here or make it simpler to
follow the code. See what makes more sense.

Regards

Marcel

[PATCH 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).

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

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

[Marcel Holtmann]

Hi Szymon,

> 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).

exactly like this please.

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

It does not make sense without the previous patches resend, but this
patch is fine with me.

Regards

Marcel

[PATCH 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>
---
 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 5/5] Bluetooth: Drop L2CAP chan reference if ERTM ack_timer fired

[Marcel Holtmann]

Hi Szymon,

> 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>
> ---
>  net/bluetooth/l2cap_core.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)

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

Regards

Marcel