We can't break userspace app that will continue using SOCK_SEQPACKET.
The default mode for SOCK_SEQPACKET is Basic Mode, so when no one
specifies the mode and sk_type is SOCK_SEQPACKET Basic Mode shall be used.
Signed-off-by: Gustavo F. Padovan <[email protected]>
---
net/bluetooth/l2cap.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index b030125..9d586fb 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -2202,7 +2202,7 @@ static int l2cap_build_conf_req(struct sock *sk, void *data)
{
struct l2cap_pinfo *pi = l2cap_pi(sk);
struct l2cap_conf_req *req = data;
- struct l2cap_conf_rfc rfc = { .mode = L2CAP_MODE_ERTM };
+ struct l2cap_conf_rfc rfc = { .mode = L2CAP_MODE_BASIC };
void *ptr = req->data;
BT_DBG("sk %p", sk);
--
1.6.3.3
Hi Gustavo,
> >> ERTM entity need to handle state vars, timers and counters to send and
> >> receive I-frames. We initialize all of them to the default values here.
> >
> > while this is a good idea. Where is the justification for pushing this
> > after the merge window?
>
> These patches are bug fixes and implementations of missing parts of ERTM spec.
> For instance, they enable ERTM to transmit in a full duplex among other things.
> Also, ERTM isn't enabled on L2CAP, so we aren't introducing any regressions and
> ASAP we put this code on mainline more feedback we can get from
> possible testers.
>
> Should I put this on the commit message?
if it fixes a bug in the current implementation, then that is fine. If
it adds another feature, then it is not acceptable. However if that
feature is mandatory by the specification, then that is a different
story. Make it perfectly clear what you are fixing or adding.
L2CAP has been in the kernel before and so the not introducing a
regression is not a real valid argument. You normally only get that for
new drivers or subsystems.
And forget about the idea with the more testers. After the merge window
that comment is void.
Regards
Marcel
Hi Marcel,
On Tue, Sep 29, 2009 at 1:55 AM, Marcel Holtmann <[email protected]> wrot=
e:
> Hi Gustavo,
>
>> ERTM entity need to handle state vars, timers and counters to send and
>> receive I-frames. We initialize all of them to the default values here.
>
> while this is a good idea. Where is the justification for pushing this
> after the merge window?
These patches are bug fixes and implementations of missing parts of ERTM sp=
ec.
For instance, they enable ERTM to transmit in a full duplex among other thi=
ngs.
Also, ERTM isn't enabled on L2CAP, so we aren't introducing any regressions=
and
ASAP we put this code on mainline more feedback we can get from
possible testers.
Should I put this on the commit message?
>
>> Signed-off-by: Gustavo F. Padovan <[email protected]>
>> ---
>> =A0net/bluetooth/l2cap.c | =A0 53 ++++++++++++++++++++++++++++++++------=
----------
>> =A01 files changed, 35 insertions(+), 18 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>> index 9d586fb..3f9d74b 100644
>> --- a/net/bluetooth/l2cap.c
>> +++ b/net/bluetooth/l2cap.c
>> @@ -2169,6 +2169,20 @@ static void l2cap_add_conf_opt(void **ptr, u8 typ=
e, u8 len, unsigned long val)
>> =A0 =A0 =A0 *ptr +=3D L2CAP_CONF_OPT_SIZE + len;
>> =A0}
>>
>> +static inline void l2cap_ertm_init(struct sock *sk) {
>> + =A0 =A0 l2cap_pi(sk)->expected_ack_seq =3D 0;
>
> Coding style! The { =A0belong on the next line.
Sorry, :(
>
> Regards
>
> Marcel
>
>
>
--=20
Gustavo F. Padovan
http://padovan.org
Hi Gustavo,
> We can't break userspace app that will continue using SOCK_SEQPACKET.
> The default mode for SOCK_SEQPACKET is Basic Mode, so when no one
> specifies the mode and sk_type is SOCK_SEQPACKET Basic Mode shall be used.
this one is fine. Applied to my tree.
Regards
Marcel
Hi Gustavo,
> SendRRorRNR need to acknowledge received I-frames so ReqSeq is set
> to BufferSeq on the the outgoing S-frame.
same here :(
Regards
Marcel
Hi Gustavo,
> ReqSeq shall be set to BufferSeq instead of ExpectedTxSeq. BufferSeq gives
> real notion of txWindow at sending side (the side sending that I-frame).
> It is incremented only when I-frames are pulled by reassembly function.
more explanation in plain English.
Regards
Marcel
Hi Gustavo,
> RejActioned is used to prevent retransmission when a entity is on the
> WAIT_F state. After send a frame with F-bit set the remote entity can
> receive I-frames again. Local L2CAP receives the F-bit and start sending
> I-frames.
same as the other one. Explain why this should be included after the
merge windows has closed.
Regards
Marcel
Hi Gustavo,
> A ERTM channel can acknowledge received I-frames by sending a I-frame with
> the proper ReqSeq value (i.e. ReqSeq is set to ExpectedTxSeq).
how does this justify for after the merge window? Explain it like I have
no idea about L2CAP ERTM.
Regards
Marcel
Hi Gustavo,
> pi->conn_state is the right place to set states
>
> Signed-off-by: Gustavo F. Padovan <[email protected]>
> ---
> net/bluetooth/l2cap.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 3f9d74b..658e27c 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -3433,7 +3433,7 @@ static inline int l2cap_data_channel_sframe(struct sock *sk, u16 rx_control, str
> } else if (rx_control & L2CAP_CTRL_FINAL) {
> if ((pi->conn_state & L2CAP_CONN_SREJ_ACT) &&
> pi->srej_save_reqseq == tx_seq)
> - pi->srej_save_reqseq &= ~L2CAP_CONN_SREJ_ACT;
> + pi->conn_state &= ~L2CAP_CONN_SREJ_ACT;
> else
> l2cap_retransmit_frame(sk, tx_seq);
> }
clearly a bug, but the commit message is not enough. Describe it in
plain English and not some cryptic way. The patch is easier to
understand the commit message. It should be the other way around.
Regards
Marcel
Hi Gustavo,
> ERTM entity need to handle state vars, timers and counters to send and
> receive I-frames. We initialize all of them to the default values here.
while this is a good idea. Where is the justification for pushing this
after the merge window?
> Signed-off-by: Gustavo F. Padovan <[email protected]>
> ---
> net/bluetooth/l2cap.c | 53 ++++++++++++++++++++++++++++++++----------------
> 1 files changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 9d586fb..3f9d74b 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -2169,6 +2169,20 @@ static void l2cap_add_conf_opt(void **ptr, u8 type, u8 len, unsigned long val)
> *ptr += L2CAP_CONF_OPT_SIZE + len;
> }
>
> +static inline void l2cap_ertm_init(struct sock *sk) {
> + l2cap_pi(sk)->expected_ack_seq = 0;
Coding style! The { belong on the next line.
Regards
Marcel
A ERTM channel can acknowledge received I-frames by sending a I-frame with
the proper ReqSeq value (i.e. ReqSeq is set to ExpectedTxSeq).
Signed-off-by: Gustavo F. Padovan <[email protected]>
---
include/net/bluetooth/l2cap.h | 1 -
net/bluetooth/l2cap.c | 8 ++++++--
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 9516f4b..327eb57 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -324,7 +324,6 @@ struct l2cap_pinfo {
__u8 next_tx_seq;
__u8 expected_ack_seq;
- __u8 req_seq;
__u8 expected_tx_seq;
__u8 buffer_seq;
__u8 buffer_seq_srej;
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 658e27c..ed245c4 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -1329,7 +1329,7 @@ static int l2cap_retransmit_frame(struct sock *sk, u8 tx_seq)
tx_skb = skb_clone(skb, GFP_ATOMIC);
bt_cb(skb)->retries++;
control = get_unaligned_le16(tx_skb->data + L2CAP_HDR_SIZE);
- control |= (pi->req_seq << L2CAP_CTRL_REQSEQ_SHIFT)
+ control |= (pi->expected_tx_seq << L2CAP_CTRL_REQSEQ_SHIFT)
| (tx_seq << L2CAP_CTRL_TXSEQ_SHIFT);
put_unaligned_le16(control, tx_skb->data + L2CAP_HDR_SIZE);
@@ -1371,7 +1371,7 @@ static int l2cap_ertm_send(struct sock *sk)
bt_cb(skb)->retries++;
control = get_unaligned_le16(tx_skb->data + L2CAP_HDR_SIZE);
- control |= (pi->req_seq << L2CAP_CTRL_REQSEQ_SHIFT)
+ control |= (pi->expected_tx_seq << L2CAP_CTRL_REQSEQ_SHIFT)
| (pi->next_tx_seq << L2CAP_CTRL_TXSEQ_SHIFT);
put_unaligned_le16(control, tx_skb->data + L2CAP_HDR_SIZE);
@@ -3288,12 +3288,16 @@ static inline int l2cap_data_channel_iframe(struct sock *sk, u16 rx_control, str
{
struct l2cap_pinfo *pi = l2cap_pi(sk);
u8 tx_seq = __get_txseq(rx_control);
+ u8 req_seq = __get_reqseq(rx_control);
u16 tx_control = 0;
u8 sar = rx_control >> L2CAP_CTRL_SAR_SHIFT;
int err = 0;
BT_DBG("sk %p rx_control 0x%4.4x len %d", sk, rx_control, skb->len);
+ pi->expected_ack_seq = req_seq;
+ l2cap_drop_acked_frames(sk);
+
if (tx_seq == pi->expected_tx_seq)
goto expected;
--
1.6.3.3
SendRRorRNR need to acknowledge received I-frames so ReqSeq is set
to BufferSeq on the the outgoing S-frame.
Signed-off-by: Gustavo F. Padovan <[email protected]>
---
net/bluetooth/l2cap.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index ddf70d6..76a7a19 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -373,6 +373,8 @@ static inline int l2cap_send_rr_or_rnr(struct l2cap_pinfo *pi, u16 control)
else
control |= L2CAP_SUPER_RCV_READY;
+ control |= pi->buffer_seq << L2CAP_CTRL_REQSEQ_SHIFT;
+
return l2cap_send_sframe(pi, control);
}
--
1.6.3.3
ReqSeq shall be set to BufferSeq instead of ExpectedTxSeq. BufferSeq gives
real notion of txWindow at sending side (the side sending that I-frame).
It is incremented only when I-frames are pulled by reassembly function.
Signed-off-by: Gustavo F. Padovan <[email protected]>
---
net/bluetooth/l2cap.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 10bf0c4..ddf70d6 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -1329,7 +1329,7 @@ static int l2cap_retransmit_frame(struct sock *sk, u8 tx_seq)
tx_skb = skb_clone(skb, GFP_ATOMIC);
bt_cb(skb)->retries++;
control = get_unaligned_le16(tx_skb->data + L2CAP_HDR_SIZE);
- control |= (pi->expected_tx_seq << L2CAP_CTRL_REQSEQ_SHIFT)
+ control |= (pi->buffer_seq << L2CAP_CTRL_REQSEQ_SHIFT)
| (tx_seq << L2CAP_CTRL_TXSEQ_SHIFT);
put_unaligned_le16(control, tx_skb->data + L2CAP_HDR_SIZE);
@@ -1371,7 +1371,7 @@ static int l2cap_ertm_send(struct sock *sk)
bt_cb(skb)->retries++;
control = get_unaligned_le16(tx_skb->data + L2CAP_HDR_SIZE);
- control |= (pi->expected_tx_seq << L2CAP_CTRL_REQSEQ_SHIFT)
+ control |= (pi->buffer_seq << L2CAP_CTRL_REQSEQ_SHIFT)
| (pi->next_tx_seq << L2CAP_CTRL_TXSEQ_SHIFT);
put_unaligned_le16(control, tx_skb->data + L2CAP_HDR_SIZE);
--
1.6.3.3
RejActioned is used to prevent retransmission when a entity is on the
WAIT_F state. After send a frame with F-bit set the remote entity can
receive I-frames again. Local L2CAP receives the F-bit and start sending
I-frames.
Signed-off-by: Gustavo F. Padovan <[email protected]>
---
include/net/bluetooth/l2cap.h | 1 +
net/bluetooth/l2cap.c | 38 +++++++++++++++++++++++++++++++++++---
2 files changed, 36 insertions(+), 3 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 327eb57..17a689f 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -374,6 +374,7 @@ struct l2cap_pinfo {
#define L2CAP_CONN_SEND_PBIT 0x10
#define L2CAP_CONN_REMOTE_BUSY 0x20
#define L2CAP_CONN_LOCAL_BUSY 0x40
+#define L2CAP_CONN_REJ_ACT 0x80
#define __mod_retrans_timer() mod_timer(&l2cap_pi(sk)->retrans_timer, \
jiffies + msecs_to_jiffies(L2CAP_DEFAULT_RETRANS_TO));
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index ed245c4..10bf0c4 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -3352,6 +3352,16 @@ expected:
return 0;
}
+ if (rx_control & L2CAP_CTRL_FINAL) {
+ if (pi->conn_state & L2CAP_CONN_REJ_ACT)
+ pi->conn_state &= ~L2CAP_CONN_REJ_ACT;
+ else {
+ sk->sk_send_head = TX_QUEUE(sk)->next;
+ pi->next_tx_seq = pi->expected_ack_seq;
+ l2cap_ertm_send(sk);
+ }
+ }
+
pi->buffer_seq = (pi->buffer_seq + 1) % 64;
err = l2cap_sar_reassembly_sdu(sk, skb, rx_control);
@@ -3388,6 +3398,14 @@ static inline int l2cap_data_channel_sframe(struct sock *sk, u16 rx_control, str
pi->expected_ack_seq = tx_seq;
l2cap_drop_acked_frames(sk);
+ if (pi->conn_state & L2CAP_CONN_REJ_ACT)
+ pi->conn_state &= ~L2CAP_CONN_REJ_ACT;
+ else {
+ sk->sk_send_head = TX_QUEUE(sk)->next;
+ pi->next_tx_seq = pi->expected_ack_seq;
+ l2cap_ertm_send(sk);
+ }
+
if (!(pi->conn_state & L2CAP_CONN_WAIT_F))
break;
@@ -3415,10 +3433,24 @@ static inline int l2cap_data_channel_sframe(struct sock *sk, u16 rx_control, str
pi->expected_ack_seq = __get_reqseq(rx_control);
l2cap_drop_acked_frames(sk);
- sk->sk_send_head = TX_QUEUE(sk)->next;
- pi->next_tx_seq = pi->expected_ack_seq;
+ if (rx_control & L2CAP_CTRL_FINAL) {
+ if (pi->conn_state & L2CAP_CONN_REJ_ACT)
+ pi->conn_state &= ~L2CAP_CONN_REJ_ACT;
+ else {
+ sk->sk_send_head = TX_QUEUE(sk)->next;
+ pi->next_tx_seq = pi->expected_ack_seq;
+ l2cap_ertm_send(sk);
+ }
+ } else {
+ sk->sk_send_head = TX_QUEUE(sk)->next;
+ pi->next_tx_seq = pi->expected_ack_seq;
+ l2cap_ertm_send(sk);
- l2cap_ertm_send(sk);
+ if (pi->conn_state & L2CAP_CONN_WAIT_F) {
+ pi->srej_save_reqseq = tx_seq;
+ pi->conn_state |= L2CAP_CONN_REJ_ACT;
+ }
+ }
break;
--
1.6.3.3
pi->conn_state is the right place to set states
Signed-off-by: Gustavo F. Padovan <[email protected]>
---
net/bluetooth/l2cap.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 3f9d74b..658e27c 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -3433,7 +3433,7 @@ static inline int l2cap_data_channel_sframe(struct sock *sk, u16 rx_control, str
} else if (rx_control & L2CAP_CTRL_FINAL) {
if ((pi->conn_state & L2CAP_CONN_SREJ_ACT) &&
pi->srej_save_reqseq == tx_seq)
- pi->srej_save_reqseq &= ~L2CAP_CONN_SREJ_ACT;
+ pi->conn_state &= ~L2CAP_CONN_SREJ_ACT;
else
l2cap_retransmit_frame(sk, tx_seq);
}
--
1.6.3.3
ERTM entity need to handle state vars, timers and counters to send and
receive I-frames. We initialize all of them to the default values here.
Signed-off-by: Gustavo F. Padovan <[email protected]>
---
net/bluetooth/l2cap.c | 53 ++++++++++++++++++++++++++++++++----------------
1 files changed, 35 insertions(+), 18 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 9d586fb..3f9d74b 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -2169,6 +2169,20 @@ static void l2cap_add_conf_opt(void **ptr, u8 type, u8 len, unsigned long val)
*ptr += L2CAP_CONF_OPT_SIZE + len;
}
+static inline void l2cap_ertm_init(struct sock *sk) {
+ l2cap_pi(sk)->expected_ack_seq = 0;
+ l2cap_pi(sk)->unacked_frames = 0;
+ l2cap_pi(sk)->buffer_seq = 0;
+ l2cap_pi(sk)->num_to_ack = 0;
+
+ setup_timer(&l2cap_pi(sk)->retrans_timer,
+ l2cap_retrans_timeout, (unsigned long) sk);
+ setup_timer(&l2cap_pi(sk)->monitor_timer,
+ l2cap_monitor_timeout, (unsigned long) sk);
+
+ __skb_queue_head_init(SREJ_QUEUE(sk));
+}
+
static int l2cap_mode_supported(__u8 mode, __u32 feat_mask)
{
u32 local_feat_mask = l2cap_feat_mask;
@@ -2752,17 +2766,13 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
sk->sk_state = BT_CONNECTED;
- l2cap_pi(sk)->next_tx_seq = 0;
- l2cap_pi(sk)->expected_ack_seq = 0;
- l2cap_pi(sk)->unacked_frames = 0;
-
- setup_timer(&l2cap_pi(sk)->retrans_timer,
- l2cap_retrans_timeout, (unsigned long) sk);
- setup_timer(&l2cap_pi(sk)->monitor_timer,
- l2cap_monitor_timeout, (unsigned long) sk);
+ l2cap_pi(sk)->next_tx_seq = 0;
+ l2cap_pi(sk)->expected_tx_seq = 0;
__skb_queue_head_init(TX_QUEUE(sk));
- __skb_queue_head_init(SREJ_QUEUE(sk));
+ if (l2cap_pi(sk)->mode == L2CAP_MODE_ERTM)
+ l2cap_ertm_init(sk);
+
l2cap_chan_ready(sk);
goto unlock;
}
@@ -2841,11 +2851,12 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;
sk->sk_state = BT_CONNECTED;
+ l2cap_pi(sk)->next_tx_seq = 0;
l2cap_pi(sk)->expected_tx_seq = 0;
- l2cap_pi(sk)->buffer_seq = 0;
- l2cap_pi(sk)->num_to_ack = 0;
__skb_queue_head_init(TX_QUEUE(sk));
- __skb_queue_head_init(SREJ_QUEUE(sk));
+ if (l2cap_pi(sk)->mode == L2CAP_MODE_ERTM)
+ l2cap_ertm_init(sk);
+
l2cap_chan_ready(sk);
}
@@ -2877,9 +2888,12 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
sk->sk_shutdown = SHUTDOWN_MASK;
skb_queue_purge(TX_QUEUE(sk));
- skb_queue_purge(SREJ_QUEUE(sk));
- del_timer(&l2cap_pi(sk)->retrans_timer);
- del_timer(&l2cap_pi(sk)->monitor_timer);
+
+ if (l2cap_pi(sk)->mode == L2CAP_MODE_ERTM) {
+ skb_queue_purge(SREJ_QUEUE(sk));
+ del_timer(&l2cap_pi(sk)->retrans_timer);
+ del_timer(&l2cap_pi(sk)->monitor_timer);
+ }
l2cap_chan_del(sk, ECONNRESET);
bh_unlock_sock(sk);
@@ -2904,9 +2918,12 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
return 0;
skb_queue_purge(TX_QUEUE(sk));
- skb_queue_purge(SREJ_QUEUE(sk));
- del_timer(&l2cap_pi(sk)->retrans_timer);
- del_timer(&l2cap_pi(sk)->monitor_timer);
+
+ if (l2cap_pi(sk)->mode == L2CAP_MODE_ERTM) {
+ skb_queue_purge(SREJ_QUEUE(sk));
+ del_timer(&l2cap_pi(sk)->retrans_timer);
+ del_timer(&l2cap_pi(sk)->monitor_timer);
+ }
l2cap_chan_del(sk, 0);
bh_unlock_sock(sk);
--
1.6.3.3