LinuxLists.cc - [RFC 0/8] ERTM state machine changes, part 2

[permalink] [raw]

Subject: Re: [RFC 5/8] Bluetooth: Move recently-added ERTM header unpacking functions

Hi Mat,

* Mat Martineau <[email protected]> [2012-04-25 16:36:16 -0700]:

> Moving these functions simplifies future patches by eliminating
> forward declarations, makes future patches easier to review, and
> better preserves 'git blame' information.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 120 ++++++++++++++++++++++----------------------
> 1 file changed, 60 insertions(+), 60 deletions(-)

Patch has been applied to bluetooth-next. Thanks.

Gustavo

2012-04-27 03:26:02

[permalink] [raw]

Subject: Re: [RFC 4/8] Bluetooth: Move recently-added ERTM header packing functions

Hi Mat,

* Mat Martineau <[email protected]> [2012-04-25 16:36:15 -0700]:

> Moving these functions simplifies future patches by eliminating
> forward declarations, makes future patches easier to review, and
> better preserves 'git blame' information.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 102 ++++++++++++++++++++++----------------------
> 1 file changed, 51 insertions(+), 51 deletions(-)

Patch has been applied to bluetooth-next. Thanks.

Gustavo

2012-04-27 03:22:16

[permalink] [raw]

Subject: Re: [RFC 3/8] Bluetooth: Remove duplicate structure members from bt_skb_cb

Hi Mat,

* Mat Martineau <[email protected]> [2012-04-25 16:36:14 -0700]:

> These values are now in the nested l2cap_ctrl struct.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> include/net/bluetooth/bluetooth.h | 3 ---
> net/bluetooth/l2cap_core.c | 34 +++++++++++++++++-----------------
> 2 files changed, 17 insertions(+), 20 deletions(-)

Patch has been applied to bluetooth-next. thanks.

Gustavo

2012-04-27 02:19:07

[permalink] [raw]

Subject: Re: [RFC 1/8] Bluetooth: Improve ERTM sequence number offset calculation

Hi Mat,

* Mat Martineau <[email protected]> [2012-04-25 16:36:12 -0700]:

> Instead of using modular division, the offset can be calculated using
> only addition and subtraction. The previous calculation did not work
> as intended and was more difficult to understand, involving unsigned
> integer underflow and a check for a negative value where one was not
> possible.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> include/net/bluetooth/l2cap.h | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)

Patch has been applied to bluetooth-next. thanks.

Gustavo

2012-04-26 23:48:42

[permalink] [raw]

Subject: Re: [RFC 6/8] Bluetooth: Lock the L2CAP channel when sending

Andrei -

On Thu, 26 Apr 2012, Andrei Emeltchenko wrote:

> Hi Mat,
>
> On Wed, Apr 25, 2012 at 04:36:17PM -0700, Mat Martineau wrote:
>> The ERTM and streaming mode transmit queue must only be accessed while
>> the L2CAP channel lock is held. Adding this lock ensures that
>> multiple threads cannot simultaneously manipulate the queue when
>> sending and receiving concurrently.
>
> I think the lock shall be added in l2cap_sock_sendmsg like:
>
> lock()
> err = l2cap_chan_send(chan, msg, len, sk->sk_priority);
> unlock()
>
> I actually use this l2cap_chan_send in A2MP code already because it gets
> locked in l2cap_data_channel.
>

You're right.

There's still a problem where the lock needs to be released when
bt_skb_send_alloc is called, because that function can block. What do
you think about releasing and reacquiring the channel lock in
l2cap_sock_alloc_skb_cb() instead?

>
>>
>> Signed-off-by: Mat Martineau <[email protected]>
>> ---
>> net/bluetooth/l2cap_core.c | 40 ++++++++++++++++++++++++++++++----------
>> 1 file changed, 30 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index 6aefeaa..8ed6a74 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -2035,26 +2035,35 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
>> u32 control;
>> int err;
>>
>> + l2cap_chan_hold(chan);
>> +
>> /* Connectionless channel */
>> if (chan->chan_type == L2CAP_CHAN_CONN_LESS) {
>> skb = l2cap_create_connless_pdu(chan, msg, len, priority);
>> - if (IS_ERR(skb))
>> - return PTR_ERR(skb);
>> + if (IS_ERR(skb)) {
>> + err = PTR_ERR(skb);
>> + goto done;
>> + }
>>
>> l2cap_do_send(chan, skb);
>> - return len;
>> + err = len;
>> + goto done;
>> }
>>
>> switch (chan->mode) {
>> case L2CAP_MODE_BASIC:
>> /* Check outgoing MTU */
>> - if (len > chan->omtu)
>> - return -EMSGSIZE;
>> + if (len > chan->omtu) {
>> + err = -EMSGSIZE;
>> + break;
>> + }
>>
>> /* Create a basic PDU */
>> skb = l2cap_create_basic_pdu(chan, msg, len, priority);
>> - if (IS_ERR(skb))
>> - return PTR_ERR(skb);
>> + if (IS_ERR(skb)) {
>> + err = PTR_ERR(skb);
>> + break;
>> + }
>>
>> l2cap_do_send(chan, skb);
>> err = len;
>> @@ -2067,30 +2076,38 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
>> control = __set_ctrl_sar(chan, L2CAP_SAR_UNSEGMENTED);
>> skb = l2cap_create_iframe_pdu(chan, msg, len, control,
>> 0);
>> - if (IS_ERR(skb))
>> - return PTR_ERR(skb);
>> + if (IS_ERR(skb)) {
>> + err = PTR_ERR(skb);
>> + break;
>> + }
>>
>> + l2cap_chan_lock(chan);
>> __skb_queue_tail(&chan->tx_q, skb);
>>
>> if (chan->tx_send_head == NULL)
>> chan->tx_send_head = skb;
>>
>> + l2cap_chan_unlock(chan);
>> } else {
>> /* Segment SDU into multiples PDUs */
>> err = l2cap_sar_segment_sdu(chan, msg, len);
>> if (err < 0)
>> - return err;
>> + break;
>> }
>>
>> if (chan->mode == L2CAP_MODE_STREAMING) {
>> + l2cap_chan_lock(chan);
>> l2cap_streaming_send(chan);
>> + l2cap_chan_unlock(chan);
>> err = len;
>> break;
>> }
>>
>> + l2cap_chan_lock(chan);
>> if (test_bit(CONN_REMOTE_BUSY, &chan->conn_state) &&
>> test_bit(CONN_WAIT_F, &chan->conn_state)) {
>> err = len;
>> + l2cap_chan_unlock(chan);
>> break;
>> }
>>
>> @@ -2098,6 +2115,7 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
>> if (err >= 0)
>> err = len;
>>
>> + l2cap_chan_unlock(chan);
>> break;
>>
>> default:
>> @@ -2105,6 +2123,8 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
>> err = -EBADFD;
>> }
>>
>> +done:
>> + l2cap_chan_put(chan);
>> return err;
>> }
>>
>> --
>> 1.7.10

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

2012-04-26 23:35:56

[permalink] [raw]

Subject: Re: [RFC 1/8] Bluetooth: Improve ERTM sequence number offset calculation

Hi Andrei -

On Thu, 26 Apr 2012, Andrei Emeltchenko wrote:

> Hi Mat,
>
> On Wed, Apr 25, 2012 at 04:36:12PM -0700, Mat Martineau wrote:
>> Instead of using modular division, the offset can be calculated using
>> only addition and subtraction. The previous calculation did not work
>> as intended and was more difficult to understand, involving unsigned
>> integer underflow and a check for a negative value where one was not
>> possible.
>
> BTW: in what cases this was not working?

"offset" was always positive, because "x % y" always gives a positive
result if y is a positive number. The 'if' clause was dead code,
which was obviously not intended.

If seq2 > seq1, then the value would wrap back around to a large
positive integer because both seq1 and seq2 are unsigned, so the value
of (seq1 - seq2) is also unsigned. Suppose seq1 is 0 and seq2 is 1.
In unsigned 16-bit integer math, 0 - 1 == 65535 due to underflow.

65535 % (63+1) is 63: luckily, the right answer for the most common tx
window (63).

65535 % (163+1) is 99: but the offset should be 163

>>
>> Signed-off-by: Mat Martineau <[email protected]>
>> ---
>> include/net/bluetooth/l2cap.h | 11 ++++-------
>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index 22e9ec9..92c0423 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -724,13 +724,10 @@ static inline bool l2cap_clear_timer(struct l2cap_chan *chan,
>>
>> static inline int __seq_offset(struct l2cap_chan *chan, __u16 seq1, __u16 seq2)
>> {
>> - int offset;
>> -
>> - offset = (seq1 - seq2) % (chan->tx_win_max + 1);
>> - if (offset < 0)
>> - offset += (chan->tx_win_max + 1);
>> -
>> - return offset;
>> + if (seq1 >= seq2)
>> + return seq1 - seq2;
>> + else
>> + return chan->tx_win_max + 1 - seq2 + seq1;
>> }
>
> You seems are changing logic, not improving as you patch states.
> Your offset might be bigger then tx_win_max. Was this intended?

The new code is correct.

Here's a python test program for all possible inputs with
tx_win_max == 63:

<code>
#!/usr/bin/env python
def seq_offset(tx_win_max, seq1, seq2):
if (seq1 >= seq2):
return seq1 - seq2
else:
return tx_win_max + 1 - seq2 + seq1

tx_win_max = 63
max_offset = -1
min_offset = tx_win_max + 1

for i in range(tx_win_max+1):
for j in range(tx_win_max+1):
offset = seq_offset(tx_win_max, i, j)
min_offset = min(offset, min_offset)
max_offset = max(offset, max_offset)

print "min: %d, max: %d" % (min_offset, max_offset)
</code>

It prints:
min: 0, max: 63

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

2012-04-26 22:03:52

[permalink] [raw]

Subject: Re: [RFC 2/8] Bluetooth: Initialize new l2cap_chan structure members

Marcel -

On Thu, 26 Apr 2012, Marcel Holtmann wrote:

> Hi Mat,
>
>> Structure members used by ERTM or streaming mode need to be
>> initialized when an ERTM or streaming mode link is configured. Some
>> duplicate code is also eliminated by moving in to the ERTM init
>> function.
>>
>> Signed-off-by: Mat Martineau <[email protected]>
>> ---
>> net/bluetooth/l2cap_core.c | 25 +++++++++++++++++--------
>> 1 file changed, 17 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index beb7194..35c0a29 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -2315,17 +2315,30 @@ static inline int l2cap_ertm_init(struct l2cap_chan *chan)
>> {
>> int err;
>>
>> + chan->next_tx_seq = 0;
>> + chan->expected_tx_seq = 0;
>> chan->expected_ack_seq = 0;
>> chan->unacked_frames = 0;
>> chan->buffer_seq = 0;
>> chan->num_acked = 0;
>> chan->frames_sent = 0;
>> + chan->last_acked_seq = 0;
>> + chan->sdu = NULL;
>> + chan->sdu_last_frag = NULL;
>> + chan->sdu_len = 0;
>> +
>> + if (chan->mode != L2CAP_MODE_ERTM)
>> + return 0;
>> +
>> + chan->rx_state = L2CAP_RX_STATE_RECV;
>> + chan->tx_state = L2CAP_TX_STATE_XMIT;
>>
>> INIT_DELAYED_WORK(&chan->retrans_timer, l2cap_retrans_timeout);
>> INIT_DELAYED_WORK(&chan->monitor_timer, l2cap_monitor_timeout);
>> INIT_DELAYED_WORK(&chan->ack_timer, l2cap_ack_timeout);
>>
>> skb_queue_head_init(&chan->srej_q);
>> + skb_queue_head_init(&chan->tx_q);
>>
>> INIT_LIST_HEAD(&chan->srej_l);
>> err = l2cap_seq_list_init(&chan->srej_list, chan->tx_win);
>> @@ -3193,10 +3206,8 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>>
>> l2cap_state_change(chan, BT_CONNECTED);
>>
>> - chan->next_tx_seq = 0;
>> - chan->expected_tx_seq = 0;
>> - skb_queue_head_init(&chan->tx_q);
>> - if (chan->mode == L2CAP_MODE_ERTM)
>> + if (chan->mode == L2CAP_MODE_ERTM ||
>> + chan->mode == L2CAP_MODE_STREAMING)
>> err = l2cap_ertm_init(chan);
>
> we need to make this compliant with the coding style the Dave Miller
> forced onto us. So either we turn this into a switch statement or we
> have to do it like this:
>
> if (chan->mode == ... ||
> chan->mode == ...)
> err = l2cap_ertm_init(...);

Ok, I had misunderstood what Dave wanted. From this message:

http://article.gmane.org/gmane.linux.network/222716

I thought he was only looking for that kind of alignment in function
calls and prototypes. However, here:

http://article.gmane.org/gmane.linux.kernel.wireless.general/85043

he mentions 'if' statements.

I will update my changes to reflect Dave's style on all long lines
with parens.

> Please keep this in mind. Otherwise we do not get the patches merged
> into the networking trees. If you wanna avoid this, I am fine using
> switch statements:
>
> switch (chan->mode) {
> case L2CAP_MODE_ERTM:
> case L2CAP_MODE_STREAMING:
> err = l2cap_ertm_init(...);
> break;
> }

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

2012-04-26 08:22:16

by Andrei Emeltchenko

[permalink] [raw]

Subject: Re: [RFC 6/8] Bluetooth: Lock the L2CAP channel when sending

Hi Mat,

On Wed, Apr 25, 2012 at 04:36:17PM -0700, Mat Martineau wrote:
> The ERTM and streaming mode transmit queue must only be accessed while
> the L2CAP channel lock is held. Adding this lock ensures that
> multiple threads cannot simultaneously manipulate the queue when
> sending and receiving concurrently.

I think the lock shall be added in l2cap_sock_sendmsg like:

lock()
err = l2cap_chan_send(chan, msg, len, sk->sk_priority);
unlock()

I actually use this l2cap_chan_send in A2MP code already because it gets
locked in l2cap_data_channel.

Best regards
Andrei Emeltchenko

>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 40 ++++++++++++++++++++++++++++++----------
> 1 file changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 6aefeaa..8ed6a74 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -2035,26 +2035,35 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
> u32 control;
> int err;
>
> + l2cap_chan_hold(chan);
> +
> /* Connectionless channel */
> if (chan->chan_type == L2CAP_CHAN_CONN_LESS) {
> skb = l2cap_create_connless_pdu(chan, msg, len, priority);
> - if (IS_ERR(skb))
> - return PTR_ERR(skb);
> + if (IS_ERR(skb)) {
> + err = PTR_ERR(skb);
> + goto done;
> + }
>
> l2cap_do_send(chan, skb);
> - return len;
> + err = len;
> + goto done;
> }
>
> switch (chan->mode) {
> case L2CAP_MODE_BASIC:
> /* Check outgoing MTU */
> - if (len > chan->omtu)
> - return -EMSGSIZE;
> + if (len > chan->omtu) {
> + err = -EMSGSIZE;
> + break;
> + }
>
> /* Create a basic PDU */
> skb = l2cap_create_basic_pdu(chan, msg, len, priority);
> - if (IS_ERR(skb))
> - return PTR_ERR(skb);
> + if (IS_ERR(skb)) {
> + err = PTR_ERR(skb);
> + break;
> + }
>
> l2cap_do_send(chan, skb);
> err = len;
> @@ -2067,30 +2076,38 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
> control = __set_ctrl_sar(chan, L2CAP_SAR_UNSEGMENTED);
> skb = l2cap_create_iframe_pdu(chan, msg, len, control,
> 0);
> - if (IS_ERR(skb))
> - return PTR_ERR(skb);
> + if (IS_ERR(skb)) {
> + err = PTR_ERR(skb);
> + break;
> + }
>
> + l2cap_chan_lock(chan);
> __skb_queue_tail(&chan->tx_q, skb);
>
> if (chan->tx_send_head == NULL)
> chan->tx_send_head = skb;
>
> + l2cap_chan_unlock(chan);
> } else {
> /* Segment SDU into multiples PDUs */
> err = l2cap_sar_segment_sdu(chan, msg, len);
> if (err < 0)
> - return err;
> + break;
> }
>
> if (chan->mode == L2CAP_MODE_STREAMING) {
> + l2cap_chan_lock(chan);
> l2cap_streaming_send(chan);
> + l2cap_chan_unlock(chan);
> err = len;
> break;
> }
>
> + l2cap_chan_lock(chan);
> if (test_bit(CONN_REMOTE_BUSY, &chan->conn_state) &&
> test_bit(CONN_WAIT_F, &chan->conn_state)) {
> err = len;
> + l2cap_chan_unlock(chan);
> break;
> }
>
> @@ -2098,6 +2115,7 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
> if (err >= 0)
> err = len;
>
> + l2cap_chan_unlock(chan);
> break;
>
> default:
> @@ -2105,6 +2123,8 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
> err = -EBADFD;
> }
>
> +done:
> + l2cap_chan_put(chan);
> return err;
> }
>
> --
> 1.7.10
>
> --
> Mat Martineau
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

2012-04-26 07:10:04

by Andrei Emeltchenko

[permalink] [raw]

Subject: Re: [RFC 1/8] Bluetooth: Improve ERTM sequence number offset calculation

Hi Mat,

On Wed, Apr 25, 2012 at 04:36:12PM -0700, Mat Martineau wrote:
> Instead of using modular division, the offset can be calculated using
> only addition and subtraction. The previous calculation did not work
> as intended and was more difficult to understand, involving unsigned
> integer underflow and a check for a negative value where one was not
> possible.

BTW: in what cases this was not working?

>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> include/net/bluetooth/l2cap.h | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 22e9ec9..92c0423 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -724,13 +724,10 @@ static inline bool l2cap_clear_timer(struct l2cap_chan *chan,
>
> static inline int __seq_offset(struct l2cap_chan *chan, __u16 seq1, __u16 seq2)
> {
> - int offset;
> -
> - offset = (seq1 - seq2) % (chan->tx_win_max + 1);
> - if (offset < 0)
> - offset += (chan->tx_win_max + 1);
> -
> - return offset;
> + if (seq1 >= seq2)
> + return seq1 - seq2;
> + else
> + return chan->tx_win_max + 1 - seq2 + seq1;
> }

You seems are changing logic, not improving as you patch states. Your offset might be bigger
then tx_win_max. Was this intended?

Best regards
Andrei Emeltchenko

2012-04-26 06:44:48

[permalink] [raw]

Subject: Re: [RFC 8/8] Bluetooth: Add Code Aurora Forum copyright

Hi Mat,

> Adding Code Aurora Forum copyright information due to significant
> additions of code.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 1 +
> 1 file changed, 1 insertion(+)

Acked-by: Marcel Holtmann <[email protected]>

Regards

Marcel

2012-04-26 06:43:32

[permalink] [raw]

Subject: Re: [RFC 7/8] Bluetooth: Refactor L2CAP ERTM and streaming transmit segmentation

Hi Mat,

> Use more common code for ERTM and streaming mode segmentation and
> transmission, and begin using skb control block data for delaying
> extended or enhanced header generation until just before the packet is
> transmitted. This code is also better suited for resegmentation,
> which is needed when L2CAP links are reconfigured after an AMP channel
> move.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> include/net/bluetooth/l2cap.h | 1 +
> net/bluetooth/l2cap_core.c | 155 +++++++++++++++++++++++------------------
> 2 files changed, 90 insertions(+), 66 deletions(-)

from what I can see, this looks good, but I like to see a second review
of this from someone else.

Regards

Marcel

2012-04-26 06:41:55

[permalink] [raw]

Subject: Re: [RFC 6/8] Bluetooth: Lock the L2CAP channel when sending

Hi Mat,

> The ERTM and streaming mode transmit queue must only be accessed while
> the L2CAP channel lock is held. Adding this lock ensures that
> multiple threads cannot simultaneously manipulate the queue when
> sending and receiving concurrently.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 40 ++++++++++++++++++++++++++++++----------
> 1 file changed, 30 insertions(+), 10 deletions(-)

Acked-by: Marcel Holtmann <[email protected]>

Regards

Marcel

2012-04-26 06:40:56

[permalink] [raw]

Subject: Re: [RFC 5/8] Bluetooth: Move recently-added ERTM header unpacking functions

Hi Mat,

> Moving these functions simplifies future patches by eliminating
> forward declarations, makes future patches easier to review, and
> better preserves 'git blame' information.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 120 ++++++++++++++++++++++----------------------
> 1 file changed, 60 insertions(+), 60 deletions(-)

Acked-by: Marcel Holtmann <[email protected]>

Regards

Marcel

2012-04-26 06:40:14

[permalink] [raw]

Subject: Re: [RFC 4/8] Bluetooth: Move recently-added ERTM header packing functions

Hi Mat,

> Moving these functions simplifies future patches by eliminating
> forward declarations, makes future patches easier to review, and
> better preserves 'git blame' information.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 102 ++++++++++++++++++++++----------------------
> 1 file changed, 51 insertions(+), 51 deletions(-)

Acked-by: Marcel Holtmann <[email protected]>

Regards

Marcel

2012-04-26 06:39:49

[permalink] [raw]

Subject: Re: [RFC 3/8] Bluetooth: Remove duplicate structure members from bt_skb_cb

Hi Mat,

> These values are now in the nested l2cap_ctrl struct.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> include/net/bluetooth/bluetooth.h | 3 ---
> net/bluetooth/l2cap_core.c | 34 +++++++++++++++++-----------------
> 2 files changed, 17 insertions(+), 20 deletions(-)

Acked-by: Marcel Holtmann <[email protected]>

Regards

Marcel

2012-04-26 06:39:25

[permalink] [raw]

Subject: Re: [RFC 1/8] Bluetooth: Improve ERTM sequence number offset calculation

Hi Mat,

> Instead of using modular division, the offset can be calculated using
> only addition and subtraction. The previous calculation did not work
> as intended and was more difficult to understand, involving unsigned
> integer underflow and a check for a negative value where one was not
> possible.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> include/net/bluetooth/l2cap.h | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)

Acked-by: Marcel Holtmann <[email protected]>

Regards

Marcel

2012-04-26 06:38:42

[permalink] [raw]

Subject: Re: [RFC 2/8] Bluetooth: Initialize new l2cap_chan structure members

Hi Mat,

> Structure members used by ERTM or streaming mode need to be
> initialized when an ERTM or streaming mode link is configured. Some
> duplicate code is also eliminated by moving in to the ERTM init
> function.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 25 +++++++++++++++++--------
> 1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index beb7194..35c0a29 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -2315,17 +2315,30 @@ static inline int l2cap_ertm_init(struct l2cap_chan *chan)
> {
> int err;
>
> + chan->next_tx_seq = 0;
> + chan->expected_tx_seq = 0;
> chan->expected_ack_seq = 0;
> chan->unacked_frames = 0;
> chan->buffer_seq = 0;
> chan->num_acked = 0;
> chan->frames_sent = 0;
> + chan->last_acked_seq = 0;
> + chan->sdu = NULL;
> + chan->sdu_last_frag = NULL;
> + chan->sdu_len = 0;
> +
> + if (chan->mode != L2CAP_MODE_ERTM)
> + return 0;
> +
> + chan->rx_state = L2CAP_RX_STATE_RECV;
> + chan->tx_state = L2CAP_TX_STATE_XMIT;
>
> INIT_DELAYED_WORK(&chan->retrans_timer, l2cap_retrans_timeout);
> INIT_DELAYED_WORK(&chan->monitor_timer, l2cap_monitor_timeout);
> INIT_DELAYED_WORK(&chan->ack_timer, l2cap_ack_timeout);
>
> skb_queue_head_init(&chan->srej_q);
> + skb_queue_head_init(&chan->tx_q);
>
> INIT_LIST_HEAD(&chan->srej_l);
> err = l2cap_seq_list_init(&chan->srej_list, chan->tx_win);
> @@ -3193,10 +3206,8 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>
> l2cap_state_change(chan, BT_CONNECTED);
>
> - chan->next_tx_seq = 0;
> - chan->expected_tx_seq = 0;
> - skb_queue_head_init(&chan->tx_q);
> - if (chan->mode == L2CAP_MODE_ERTM)
> + if (chan->mode == L2CAP_MODE_ERTM ||
> + chan->mode == L2CAP_MODE_STREAMING)
> err = l2cap_ertm_init(chan);

we need to make this compliant with the coding style the Dave Miller
forced onto us. So either we turn this into a switch statement or we
have to do it like this:

if (chan->mode == ... ||
chan->mode == ...)
err = l2cap_ertm_init(...);

Please keep this in mind. Otherwise we do not get the patches merged
into the networking trees. If you wanna avoid this, I am fine using
switch statements:

switch (chan->mode) {
case L2CAP_MODE_ERTM:
case L2CAP_MODE_STREAMING:
err = l2cap_ertm_init(...);
break;
}

Regards

Marcel

2012-04-25 23:36:18

[permalink] [raw]

Subject: [RFC 7/8] Bluetooth: Refactor L2CAP ERTM and streaming transmit segmentation

Use more common code for ERTM and streaming mode segmentation and
transmission, and begin using skb control block data for delaying
extended or enhanced header generation until just before the packet is
transmitted. This code is also better suited for resegmentation,
which is needed when L2CAP links are reconfigured after an AMP channel
move.

Signed-off-by: Mat Martineau <[email protected]>
---
include/net/bluetooth/l2cap.h | 1 +
net/bluetooth/l2cap_core.c | 155 +++++++++++++++++++++++------------------
2 files changed, 90 insertions(+), 66 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 92c0423..88c128a 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -44,6 +44,7 @@
#define L2CAP_DEFAULT_MAX_SDU_SIZE 0xFFFF
#define L2CAP_DEFAULT_SDU_ITIME 0xFFFFFFFF
#define L2CAP_DEFAULT_ACC_LAT 0xFFFFFFFF
+#define L2CAP_BREDR_MAX_PAYLOAD 1019 /* 3-DH5 packet */

#define L2CAP_DISC_TIMEOUT msecs_to_jiffies(100)
#define L2CAP_DISC_REJ_TIMEOUT msecs_to_jiffies(5000)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 8ed6a74..da7c38a 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1643,6 +1643,7 @@ static void l2cap_streaming_send(struct l2cap_chan *chan)
while ((skb = skb_dequeue(&chan->tx_q))) {
control = __get_control(chan, skb->data + L2CAP_HDR_SIZE);
control |= __set_txseq(chan, chan->next_tx_seq);
+ control |= __set_ctrl_sar(chan, bt_cb(skb)->control.sar);
__put_control(chan, control, skb->data + L2CAP_HDR_SIZE);

if (chan->fcs == L2CAP_FCS_CRC16) {
@@ -1715,6 +1716,9 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
if (chan->state != BT_CONNECTED)
return -ENOTCONN;

+ if (test_bit(CONN_REMOTE_BUSY, &chan->conn_state))
+ return 0;
+
while ((skb = chan->tx_send_head) && (!l2cap_tx_window_full(chan))) {

if (chan->remote_max_tx &&
@@ -1735,6 +1739,7 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)

control |= __set_reqseq(chan, chan->buffer_seq);
control |= __set_txseq(chan, chan->next_tx_seq);
+ control |= __set_ctrl_sar(chan, bt_cb(skb)->control.sar);

__put_control(chan, control, tx_skb->data + L2CAP_HDR_SIZE);

@@ -1930,7 +1935,7 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,

static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
struct msghdr *msg, size_t len,
- u32 control, u16 sdulen)
+ u16 sdulen)
{
struct l2cap_conn *conn = chan->conn;
struct sk_buff *skb;
@@ -1965,7 +1970,7 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
lh->cid = cpu_to_le16(chan->dcid);
lh->len = cpu_to_le16(len + (hlen - L2CAP_HDR_SIZE));

- __put_control(chan, control, skb_put(skb, __ctrl_size(chan)));
+ __put_control(chan, 0, skb_put(skb, __ctrl_size(chan)));

if (sdulen)
put_unaligned_le16(sdulen, skb_put(skb, L2CAP_SDULEN_SIZE));
@@ -1983,57 +1988,78 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
return skb;
}

-static int l2cap_sar_segment_sdu(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
+static int l2cap_segment_sdu(struct l2cap_chan *chan,
+ struct sk_buff_head *seg_queue,
+ struct msghdr *msg, size_t len)
{
struct sk_buff *skb;
- struct sk_buff_head sar_queue;
- u32 control;
- size_t size = 0;
+ u16 sdu_len;
+ size_t pdu_len;
+ int err = 0;
+ u8 sar;

- skb_queue_head_init(&sar_queue);
- control = __set_ctrl_sar(chan, L2CAP_SAR_START);
- skb = l2cap_create_iframe_pdu(chan, msg, chan->remote_mps, control, len);
- if (IS_ERR(skb))
- return PTR_ERR(skb);
+ BT_DBG("chan %p, msg %p, len %d", chan, msg, (int)len);

- __skb_queue_tail(&sar_queue, skb);
- len -= chan->remote_mps;
- size += chan->remote_mps;
+ /* It is critical that ERTM PDUs fit in a single HCI fragment,
+ * so fragmented skbs are not used. The HCI layer's handling
+ * of fragmented skbs is not compatible with ERTM's queueing.
+ */

- while (len > 0) {
- size_t buflen;
+ /* PDU size is derived from the HCI MTU */
+ pdu_len = chan->conn->mtu;

- if (len > chan->remote_mps) {
- control = __set_ctrl_sar(chan, L2CAP_SAR_CONTINUE);
- buflen = chan->remote_mps;
- } else {
- control = __set_ctrl_sar(chan, L2CAP_SAR_END);
- buflen = len;
- }
+ pdu_len = min_t(size_t, pdu_len, L2CAP_BREDR_MAX_PAYLOAD);
+
+ /* Adjust for largest possible L2CAP overhead. */
+ pdu_len -= L2CAP_EXT_HDR_SIZE + L2CAP_FCS_SIZE;
+
+ /* Remote device may have requested smaller PDUs */
+ pdu_len = min_t(size_t, pdu_len, chan->remote_mps);
+
+ if (len <= pdu_len) {
+ sar = L2CAP_SAR_UNSEGMENTED;
+ sdu_len = 0;
+ pdu_len = len;
+ } else {
+ sar = L2CAP_SAR_START;
+ sdu_len = len;
+ pdu_len -= L2CAP_SDULEN_SIZE;
+ }
+
+ while (len > 0) {
+ skb = l2cap_create_iframe_pdu(chan, msg, pdu_len, sdu_len);

- skb = l2cap_create_iframe_pdu(chan, msg, buflen, control, 0);
if (IS_ERR(skb)) {
- skb_queue_purge(&sar_queue);
+ __skb_queue_purge(seg_queue);
return PTR_ERR(skb);
}

- __skb_queue_tail(&sar_queue, skb);
- len -= buflen;
- size += buflen;
+ bt_cb(skb)->control.sar = sar;
+ __skb_queue_tail(seg_queue, skb);
+
+ len -= pdu_len;
+ if (sdu_len) {
+ sdu_len = 0;
+ pdu_len += L2CAP_SDULEN_SIZE;
+ }
+
+ if (len <= pdu_len) {
+ sar = L2CAP_SAR_END;
+ pdu_len = len;
+ } else {
+ sar = L2CAP_SAR_CONTINUE;
+ }
}
- skb_queue_splice_tail(&sar_queue, &chan->tx_q);
- if (chan->tx_send_head == NULL)
- chan->tx_send_head = sar_queue.next;

- return size;
+ return err;
}

int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
u32 priority)
{
struct sk_buff *skb;
- u32 control;
int err;
+ struct sk_buff_head seg_queue;

l2cap_chan_hold(chan);

@@ -2071,51 +2097,48 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,

case L2CAP_MODE_ERTM:
case L2CAP_MODE_STREAMING:
- /* Entire SDU fits into one PDU */
- if (len <= chan->remote_mps) {
- control = __set_ctrl_sar(chan, L2CAP_SAR_UNSEGMENTED);
- skb = l2cap_create_iframe_pdu(chan, msg, len, control,
- 0);
- if (IS_ERR(skb)) {
- err = PTR_ERR(skb);
- break;
- }
+ /* Check outgoing MTU */
+ if (len > chan->omtu) {
+ err = -EMSGSIZE;
+ break;
+ }

- l2cap_chan_lock(chan);
- __skb_queue_tail(&chan->tx_q, skb);
+ __skb_queue_head_init(&seg_queue);

- if (chan->tx_send_head == NULL)
- chan->tx_send_head = skb;
+ /* Do segmentation before calling in to the state machine,
+ * since it's possible to block while waiting for memory
+ * allocation.
+ */
+ err = l2cap_segment_sdu(chan, &seg_queue, msg, len);

- l2cap_chan_unlock(chan);
- } else {
- /* Segment SDU into multiples PDUs */
- err = l2cap_sar_segment_sdu(chan, msg, len);
- if (err < 0)
- break;
+ /* The channel could have been closed while segmenting,
+ * check that it is still connected.
+ */
+ if (chan->state != BT_CONNECTED) {
+ __skb_queue_purge(&seg_queue);
+ err = -ENOTCONN;
}

- if (chan->mode == L2CAP_MODE_STREAMING) {
- l2cap_chan_lock(chan);
- l2cap_streaming_send(chan);
- l2cap_chan_unlock(chan);
- err = len;
+ if (err)
break;
- }

l2cap_chan_lock(chan);
- if (test_bit(CONN_REMOTE_BUSY, &chan->conn_state) &&
- test_bit(CONN_WAIT_F, &chan->conn_state)) {
- err = len;
- l2cap_chan_unlock(chan);
- break;
- }

- err = l2cap_ertm_send(chan);
- if (err >= 0)
+ skb_queue_splice_tail_init(&seg_queue, &chan->tx_q);
+ if (chan->mode == L2CAP_MODE_ERTM)
+ err = l2cap_ertm_send(chan);
+ else
+ l2cap_streaming_send(chan);
+
+ if (!err)
err = len;

l2cap_chan_unlock(chan);
+
+ /* If the skbs were not queued for sending, they'll still be in
+ * seg_queue and need to be purged.
+ */
+ __skb_queue_purge(&seg_queue);
break;

default:
--
1.7.10

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

2012-04-25 23:36:19

[permalink] [raw]

Subject: [RFC 8/8] Bluetooth: Add Code Aurora Forum copyright

Adding Code Aurora Forum copyright information due to significant
additions of code.

Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap_core.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index da7c38a..8d09d56 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4,6 +4,7 @@
Copyright (C) 2009-2010 Gustavo F. Padovan <[email protected]>
Copyright (C) 2010 Google Inc.
Copyright (C) 2011 ProFUSION Embedded Systems
+ Copyright (c) 2012 Code Aurora Forum. All rights reserved.

Written 2000,2001 by Maxim Krasnyansky <[email protected]>

--
1.7.10

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

2012-04-25 23:36:16

[permalink] [raw]

Subject: [RFC 5/8] Bluetooth: Move recently-added ERTM header unpacking functions

Moving these functions simplifies future patches by eliminating
forward declarations, makes future patches easier to review, and
better preserves 'git blame' information.

Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap_core.c | 120 ++++++++++++++++++++++----------------------
1 file changed, 60 insertions(+), 60 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 3221f17..6aefeaa 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -725,6 +725,66 @@ static void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb)
hci_send_acl(chan->conn->hchan, skb, flags);
}

+static void __unpack_enhanced_control(u16 enh, struct l2cap_ctrl *control)
+{
+ control->reqseq = (enh & L2CAP_CTRL_REQSEQ) >> L2CAP_CTRL_REQSEQ_SHIFT;
+ control->final = (enh & L2CAP_CTRL_FINAL) >> L2CAP_CTRL_FINAL_SHIFT;
+
+ if (enh & L2CAP_CTRL_FRAME_TYPE) {
+ /* S-Frame */
+ control->sframe = 1;
+ control->poll = (enh & L2CAP_CTRL_POLL) >> L2CAP_CTRL_POLL_SHIFT;
+ control->super = (enh & L2CAP_CTRL_SUPERVISE) >> L2CAP_CTRL_SUPER_SHIFT;
+
+ control->sar = 0;
+ control->txseq = 0;
+ } else {
+ /* I-Frame */
+ control->sframe = 0;
+ control->sar = (enh & L2CAP_CTRL_SAR) >> L2CAP_CTRL_SAR_SHIFT;
+ control->txseq = (enh & L2CAP_CTRL_TXSEQ) >> L2CAP_CTRL_TXSEQ_SHIFT;
+
+ control->poll = 0;
+ control->super = 0;
+ }
+}
+
+static void __unpack_extended_control(u32 ext, struct l2cap_ctrl *control)
+{
+ control->reqseq = (ext & L2CAP_EXT_CTRL_REQSEQ) >> L2CAP_EXT_CTRL_REQSEQ_SHIFT;
+ control->final = (ext & L2CAP_EXT_CTRL_FINAL) >> L2CAP_EXT_CTRL_FINAL_SHIFT;
+
+ if (ext & L2CAP_EXT_CTRL_FRAME_TYPE) {
+ /* S-Frame */
+ control->sframe = 1;
+ control->poll = (ext & L2CAP_EXT_CTRL_POLL) >> L2CAP_EXT_CTRL_POLL_SHIFT;
+ control->super = (ext & L2CAP_EXT_CTRL_SUPERVISE) >> L2CAP_EXT_CTRL_SUPER_SHIFT;
+
+ control->sar = 0;
+ control->txseq = 0;
+ } else {
+ /* I-Frame */
+ control->sframe = 0;
+ control->sar = (ext & L2CAP_EXT_CTRL_SAR) >> L2CAP_EXT_CTRL_SAR_SHIFT;
+ control->txseq = (ext & L2CAP_EXT_CTRL_TXSEQ) >> L2CAP_EXT_CTRL_TXSEQ_SHIFT;
+
+ control->poll = 0;
+ control->super = 0;
+ }
+}
+
+static inline void __unpack_control(struct l2cap_chan *chan,
+ struct sk_buff *skb)
+{
+ if (test_bit(FLAG_EXT_CTRL, &chan->flags)) {
+ __unpack_extended_control(get_unaligned_le32(skb->data),
+ &bt_cb(skb)->control);
+ } else {
+ __unpack_enhanced_control(get_unaligned_le16(skb->data),
+ &bt_cb(skb)->control);
+ }
+}
+
static u32 __pack_extended_control(struct l2cap_ctrl *control)
{
u32 packed;
@@ -838,66 +898,6 @@ static inline void l2cap_send_rr_or_rnr(struct l2cap_chan *chan, u32 control)
l2cap_send_sframe(chan, control);
}

-static void __unpack_enhanced_control(u16 enh, struct l2cap_ctrl *control)
-{
- control->reqseq = (enh & L2CAP_CTRL_REQSEQ) >> L2CAP_CTRL_REQSEQ_SHIFT;
- control->final = (enh & L2CAP_CTRL_FINAL) >> L2CAP_CTRL_FINAL_SHIFT;
-
- if (enh & L2CAP_CTRL_FRAME_TYPE) {
- /* S-Frame */
- control->sframe = 1;
- control->poll = (enh & L2CAP_CTRL_POLL) >> L2CAP_CTRL_POLL_SHIFT;
- control->super = (enh & L2CAP_CTRL_SUPERVISE) >> L2CAP_CTRL_SUPER_SHIFT;
-
- control->sar = 0;
- control->txseq = 0;
- } else {
- /* I-Frame */
- control->sframe = 0;
- control->sar = (enh & L2CAP_CTRL_SAR) >> L2CAP_CTRL_SAR_SHIFT;
- control->txseq = (enh & L2CAP_CTRL_TXSEQ) >> L2CAP_CTRL_TXSEQ_SHIFT;
-
- control->poll = 0;
- control->super = 0;
- }
-}
-
-static void __unpack_extended_control(u32 ext, struct l2cap_ctrl *control)
-{
- control->reqseq = (ext & L2CAP_EXT_CTRL_REQSEQ) >> L2CAP_EXT_CTRL_REQSEQ_SHIFT;
- control->final = (ext & L2CAP_EXT_CTRL_FINAL) >> L2CAP_EXT_CTRL_FINAL_SHIFT;
-
- if (ext & L2CAP_EXT_CTRL_FRAME_TYPE) {
- /* S-Frame */
- control->sframe = 1;
- control->poll = (ext & L2CAP_EXT_CTRL_POLL) >> L2CAP_EXT_CTRL_POLL_SHIFT;
- control->super = (ext & L2CAP_EXT_CTRL_SUPERVISE) >> L2CAP_EXT_CTRL_SUPER_SHIFT;
-
- control->sar = 0;
- control->txseq = 0;
- } else {
- /* I-Frame */
- control->sframe = 0;
- control->sar = (ext & L2CAP_EXT_CTRL_SAR) >> L2CAP_EXT_CTRL_SAR_SHIFT;
- control->txseq = (ext & L2CAP_EXT_CTRL_TXSEQ) >> L2CAP_EXT_CTRL_TXSEQ_SHIFT;
-
- control->poll = 0;
- control->super = 0;
- }
-}
-
-static inline void __unpack_control(struct l2cap_chan *chan,
- struct sk_buff *skb)
-{
- if (test_bit(FLAG_EXT_CTRL, &chan->flags)) {
- __unpack_extended_control(get_unaligned_le32(skb->data),
- &bt_cb(skb)->control);
- } else {
- __unpack_enhanced_control(get_unaligned_le16(skb->data),
- &bt_cb(skb)->control);
- }
-}
-
static inline int __l2cap_no_conn_pending(struct l2cap_chan *chan)
{
return !test_bit(CONF_CONNECT_PEND, &chan->conf_state);
--
1.7.10

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

2012-04-25 23:36:17

[permalink] [raw]

Subject: [RFC 6/8] Bluetooth: Lock the L2CAP channel when sending

The ERTM and streaming mode transmit queue must only be accessed while
the L2CAP channel lock is held. Adding this lock ensures that
multiple threads cannot simultaneously manipulate the queue when
sending and receiving concurrently.

Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap_core.c | 40 ++++++++++++++++++++++++++++++----------
1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 6aefeaa..8ed6a74 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2035,26 +2035,35 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
u32 control;
int err;

+ l2cap_chan_hold(chan);
+
/* Connectionless channel */
if (chan->chan_type == L2CAP_CHAN_CONN_LESS) {
skb = l2cap_create_connless_pdu(chan, msg, len, priority);
- if (IS_ERR(skb))
- return PTR_ERR(skb);
+ if (IS_ERR(skb)) {
+ err = PTR_ERR(skb);
+ goto done;
+ }

l2cap_do_send(chan, skb);
- return len;
+ err = len;
+ goto done;
}

switch (chan->mode) {
case L2CAP_MODE_BASIC:
/* Check outgoing MTU */
- if (len > chan->omtu)
- return -EMSGSIZE;
+ if (len > chan->omtu) {
+ err = -EMSGSIZE;
+ break;
+ }

/* Create a basic PDU */
skb = l2cap_create_basic_pdu(chan, msg, len, priority);
- if (IS_ERR(skb))
- return PTR_ERR(skb);
+ if (IS_ERR(skb)) {
+ err = PTR_ERR(skb);
+ break;
+ }

l2cap_do_send(chan, skb);
err = len;
@@ -2067,30 +2076,38 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
control = __set_ctrl_sar(chan, L2CAP_SAR_UNSEGMENTED);
skb = l2cap_create_iframe_pdu(chan, msg, len, control,
0);
- if (IS_ERR(skb))
- return PTR_ERR(skb);
+ if (IS_ERR(skb)) {
+ err = PTR_ERR(skb);
+ break;
+ }

+ l2cap_chan_lock(chan);
__skb_queue_tail(&chan->tx_q, skb);

if (chan->tx_send_head == NULL)
chan->tx_send_head = skb;

+ l2cap_chan_unlock(chan);
} else {
/* Segment SDU into multiples PDUs */
err = l2cap_sar_segment_sdu(chan, msg, len);
if (err < 0)
- return err;
+ break;
}

if (chan->mode == L2CAP_MODE_STREAMING) {
+ l2cap_chan_lock(chan);
l2cap_streaming_send(chan);
+ l2cap_chan_unlock(chan);
err = len;
break;
}

+ l2cap_chan_lock(chan);
if (test_bit(CONN_REMOTE_BUSY, &chan->conn_state) &&
test_bit(CONN_WAIT_F, &chan->conn_state)) {
err = len;
+ l2cap_chan_unlock(chan);
break;
}

@@ -2098,6 +2115,7 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
if (err >= 0)
err = len;

+ l2cap_chan_unlock(chan);
break;

default:
@@ -2105,6 +2123,8 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
err = -EBADFD;
}

+done:
+ l2cap_chan_put(chan);
return err;
}

--
1.7.10

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

2012-04-25 23:36:15

[permalink] [raw]

Subject: [RFC 4/8] Bluetooth: Move recently-added ERTM header packing functions

Moving these functions simplifies future patches by eliminating
forward declarations, makes future patches easier to review, and
better preserves 'git blame' information.

Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap_core.c | 102 ++++++++++++++++++++++----------------------
1 file changed, 51 insertions(+), 51 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 9a33f21..3221f17 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -725,6 +725,57 @@ static void l2cap_do_send(struct l2cap_chan *chan, struct sk_buff *skb)
hci_send_acl(chan->conn->hchan, skb, flags);
}

+static u32 __pack_extended_control(struct l2cap_ctrl *control)
+{
+ u32 packed;
+
+ packed = control->reqseq << L2CAP_EXT_CTRL_REQSEQ_SHIFT;
+ packed |= control->final << L2CAP_EXT_CTRL_FINAL_SHIFT;
+
+ if (control->sframe) {
+ packed |= control->poll << L2CAP_EXT_CTRL_POLL_SHIFT;
+ packed |= control->super << L2CAP_EXT_CTRL_SUPER_SHIFT;
+ packed |= L2CAP_EXT_CTRL_FRAME_TYPE;
+ } else {
+ packed |= control->sar << L2CAP_EXT_CTRL_SAR_SHIFT;
+ packed |= control->txseq << L2CAP_EXT_CTRL_TXSEQ_SHIFT;
+ }
+
+ return packed;
+}
+
+static u16 __pack_enhanced_control(struct l2cap_ctrl *control)
+{
+ u16 packed;
+
+ packed = control->reqseq << L2CAP_CTRL_REQSEQ_SHIFT;
+ packed |= control->final << L2CAP_CTRL_FINAL_SHIFT;
+
+ if (control->sframe) {
+ packed |= control->poll << L2CAP_CTRL_POLL_SHIFT;
+ packed |= control->super << L2CAP_CTRL_SUPER_SHIFT;
+ packed |= L2CAP_CTRL_FRAME_TYPE;
+ } else {
+ packed |= control->sar << L2CAP_CTRL_SAR_SHIFT;
+ packed |= control->txseq << L2CAP_CTRL_TXSEQ_SHIFT;
+ }
+
+ return packed;
+}
+
+static inline void __pack_control(struct l2cap_chan *chan,
+ struct l2cap_ctrl *control,
+ struct sk_buff *skb)
+{
+ if (test_bit(FLAG_EXT_CTRL, &chan->flags)) {
+ put_unaligned_le32(__pack_extended_control(control),
+ skb->data + L2CAP_HDR_SIZE);
+ } else {
+ put_unaligned_le16(__pack_enhanced_control(control),
+ skb->data + L2CAP_HDR_SIZE);
+ }
+}
+
static inline void l2cap_send_sframe(struct l2cap_chan *chan, u32 control)
{
struct sk_buff *skb;
@@ -787,25 +838,6 @@ static inline void l2cap_send_rr_or_rnr(struct l2cap_chan *chan, u32 control)
l2cap_send_sframe(chan, control);
}

-static u16 __pack_enhanced_control(struct l2cap_ctrl *control)
-{
- u16 packed;
-
- packed = control->reqseq << L2CAP_CTRL_REQSEQ_SHIFT;
- packed |= control->final << L2CAP_CTRL_FINAL_SHIFT;
-
- if (control->sframe) {
- packed |= control->poll << L2CAP_CTRL_POLL_SHIFT;
- packed |= control->super << L2CAP_CTRL_SUPER_SHIFT;
- packed |= L2CAP_CTRL_FRAME_TYPE;
- } else {
- packed |= control->sar << L2CAP_CTRL_SAR_SHIFT;
- packed |= control->txseq << L2CAP_CTRL_TXSEQ_SHIFT;
- }
-
- return packed;
-}
-
static void __unpack_enhanced_control(u16 enh, struct l2cap_ctrl *control)
{
control->reqseq = (enh & L2CAP_CTRL_REQSEQ) >> L2CAP_CTRL_REQSEQ_SHIFT;
@@ -830,25 +862,6 @@ static void __unpack_enhanced_control(u16 enh, struct l2cap_ctrl *control)
}
}

-static u32 __pack_extended_control(struct l2cap_ctrl *control)
-{
- u32 packed;
-
- packed = control->reqseq << L2CAP_EXT_CTRL_REQSEQ_SHIFT;
- packed |= control->final << L2CAP_EXT_CTRL_FINAL_SHIFT;
-
- if (control->sframe) {
- packed |= control->poll << L2CAP_EXT_CTRL_POLL_SHIFT;
- packed |= control->super << L2CAP_EXT_CTRL_SUPER_SHIFT;
- packed |= L2CAP_EXT_CTRL_FRAME_TYPE;
- } else {
- packed |= control->sar << L2CAP_EXT_CTRL_SAR_SHIFT;
- packed |= control->txseq << L2CAP_EXT_CTRL_TXSEQ_SHIFT;
- }
-
- return packed;
-}
-
static void __unpack_extended_control(u32 ext, struct l2cap_ctrl *control)
{
control->reqseq = (ext & L2CAP_EXT_CTRL_REQSEQ) >> L2CAP_EXT_CTRL_REQSEQ_SHIFT;
@@ -885,19 +898,6 @@ static inline void __unpack_control(struct l2cap_chan *chan,
}
}

-static inline void __pack_control(struct l2cap_chan *chan,
- struct l2cap_ctrl *control,
- struct sk_buff *skb)
-{
- if (test_bit(FLAG_EXT_CTRL, &chan->flags)) {
- put_unaligned_le32(__pack_extended_control(control),
- skb->data + L2CAP_HDR_SIZE);
- } else {
- put_unaligned_le16(__pack_enhanced_control(control),
- skb->data + L2CAP_HDR_SIZE);
- }
-}
-
static inline int __l2cap_no_conn_pending(struct l2cap_chan *chan)
{
return !test_bit(CONF_CONNECT_PEND, &chan->conf_state);
--
1.7.10

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

2012-04-25 23:36:13

[permalink] [raw]

Subject: [RFC 2/8] Bluetooth: Initialize new l2cap_chan structure members

Structure members used by ERTM or streaming mode need to be
initialized when an ERTM or streaming mode link is configured. Some
duplicate code is also eliminated by moving in to the ERTM init
function.

Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap_core.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index beb7194..35c0a29 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2315,17 +2315,30 @@ static inline int l2cap_ertm_init(struct l2cap_chan *chan)
{
int err;

+ chan->next_tx_seq = 0;
+ chan->expected_tx_seq = 0;
chan->expected_ack_seq = 0;
chan->unacked_frames = 0;
chan->buffer_seq = 0;
chan->num_acked = 0;
chan->frames_sent = 0;
+ chan->last_acked_seq = 0;
+ chan->sdu = NULL;
+ chan->sdu_last_frag = NULL;
+ chan->sdu_len = 0;
+
+ if (chan->mode != L2CAP_MODE_ERTM)
+ return 0;
+
+ chan->rx_state = L2CAP_RX_STATE_RECV;
+ chan->tx_state = L2CAP_TX_STATE_XMIT;

INIT_DELAYED_WORK(&chan->retrans_timer, l2cap_retrans_timeout);
INIT_DELAYED_WORK(&chan->monitor_timer, l2cap_monitor_timeout);
INIT_DELAYED_WORK(&chan->ack_timer, l2cap_ack_timeout);

skb_queue_head_init(&chan->srej_q);
+ skb_queue_head_init(&chan->tx_q);

INIT_LIST_HEAD(&chan->srej_l);
err = l2cap_seq_list_init(&chan->srej_list, chan->tx_win);
@@ -3193,10 +3206,8 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr

l2cap_state_change(chan, BT_CONNECTED);

- chan->next_tx_seq = 0;
- chan->expected_tx_seq = 0;
- skb_queue_head_init(&chan->tx_q);
- if (chan->mode == L2CAP_MODE_ERTM)
+ if (chan->mode == L2CAP_MODE_ERTM ||
+ chan->mode == L2CAP_MODE_STREAMING)
err = l2cap_ertm_init(chan);

if (err < 0)
@@ -3328,10 +3339,8 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
set_default_fcs(chan);

l2cap_state_change(chan, BT_CONNECTED);
- chan->next_tx_seq = 0;
- chan->expected_tx_seq = 0;
- skb_queue_head_init(&chan->tx_q);
- if (chan->mode == L2CAP_MODE_ERTM)
+ if (chan->mode == L2CAP_MODE_ERTM ||
+ chan->mode == L2CAP_MODE_STREAMING)
err = l2cap_ertm_init(chan);

if (err < 0)
--
1.7.10

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

2012-04-25 23:36:14

[permalink] [raw]

Subject: [RFC 3/8] Bluetooth: Remove duplicate structure members from bt_skb_cb

These values are now in the nested l2cap_ctrl struct.

Signed-off-by: Mat Martineau <[email protected]>
---
include/net/bluetooth/bluetooth.h | 3 ---
net/bluetooth/l2cap_core.c | 34 +++++++++++++++++-----------------
2 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 27a6a93..2fb268f 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -235,9 +235,6 @@ struct bt_skb_cb {
__u8 pkt_type;
__u8 incoming;
__u16 expect;
- __u16 tx_seq;
- __u8 retries;
- __u8 sar;
__u8 force_active;
struct l2cap_ctrl control;
};
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 35c0a29..9a33f21 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1621,7 +1621,7 @@ static void l2cap_drop_acked_frames(struct l2cap_chan *chan)

while ((skb = skb_peek(&chan->tx_q)) &&
chan->unacked_frames) {
- if (bt_cb(skb)->tx_seq == chan->expected_ack_seq)
+ if (bt_cb(skb)->control.txseq == chan->expected_ack_seq)
break;

skb = skb_dequeue(&chan->tx_q);
@@ -1668,7 +1668,7 @@ static void l2cap_retransmit_one_frame(struct l2cap_chan *chan, u16 tx_seq)
if (!skb)
return;

- while (bt_cb(skb)->tx_seq != tx_seq) {
+ while (bt_cb(skb)->control.txseq != tx_seq) {
if (skb_queue_is_last(&chan->tx_q, skb))
return;

@@ -1676,13 +1676,13 @@ static void l2cap_retransmit_one_frame(struct l2cap_chan *chan, u16 tx_seq)
}

if (chan->remote_max_tx &&
- bt_cb(skb)->retries == chan->remote_max_tx) {
+ bt_cb(skb)->control.retries == chan->remote_max_tx) {
l2cap_send_disconn_req(chan->conn, chan, ECONNABORTED);
return;
}

tx_skb = skb_clone(skb, GFP_ATOMIC);
- bt_cb(skb)->retries++;
+ bt_cb(skb)->control.retries++;

control = __get_control(chan, tx_skb->data + L2CAP_HDR_SIZE);
control &= __get_sar_mask(chan);
@@ -1718,14 +1718,14 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
while ((skb = chan->tx_send_head) && (!l2cap_tx_window_full(chan))) {

if (chan->remote_max_tx &&
- bt_cb(skb)->retries == chan->remote_max_tx) {
+ bt_cb(skb)->control.retries == chan->remote_max_tx) {
l2cap_send_disconn_req(chan->conn, chan, ECONNABORTED);
break;
}

tx_skb = skb_clone(skb, GFP_ATOMIC);

- bt_cb(skb)->retries++;
+ bt_cb(skb)->control.retries++;

control = __get_control(chan, tx_skb->data + L2CAP_HDR_SIZE);
control &= __get_sar_mask(chan);
@@ -1749,11 +1749,11 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)

__set_retrans_timer(chan);

- bt_cb(skb)->tx_seq = chan->next_tx_seq;
+ bt_cb(skb)->control.txseq = chan->next_tx_seq;

chan->next_tx_seq = __next_seq(chan, chan->next_tx_seq);

- if (bt_cb(skb)->retries == 1) {
+ if (bt_cb(skb)->control.retries == 1) {
chan->unacked_frames++;

if (!nsent++)
@@ -1979,7 +1979,7 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
if (chan->fcs == L2CAP_FCS_CRC16)
put_unaligned_le16(0, skb_put(skb, L2CAP_FCS_SIZE));

- bt_cb(skb)->retries = 0;
+ bt_cb(skb)->control.retries = 0;
return skb;
}

@@ -3960,19 +3960,19 @@ static int l2cap_add_to_srej_queue(struct l2cap_chan *chan, struct sk_buff *skb,
struct sk_buff *next_skb;
int tx_seq_offset, next_tx_seq_offset;

- bt_cb(skb)->tx_seq = tx_seq;
- bt_cb(skb)->sar = sar;
+ bt_cb(skb)->control.txseq = tx_seq;
+ bt_cb(skb)->control.sar = sar;

next_skb = skb_peek(&chan->srej_q);

tx_seq_offset = __seq_offset(chan, tx_seq, chan->buffer_seq);

while (next_skb) {
- if (bt_cb(next_skb)->tx_seq == tx_seq)
+ if (bt_cb(next_skb)->control.txseq == tx_seq)
return -EINVAL;

next_tx_seq_offset = __seq_offset(chan,
- bt_cb(next_skb)->tx_seq, chan->buffer_seq);
+ bt_cb(next_skb)->control.txseq, chan->buffer_seq);

if (next_tx_seq_offset > tx_seq_offset) {
__skb_queue_before(&chan->srej_q, next_skb, skb);
@@ -4144,11 +4144,11 @@ static void l2cap_check_srej_gap(struct l2cap_chan *chan, u16 tx_seq)
!test_bit(CONN_LOCAL_BUSY, &chan->conn_state)) {
int err;

- if (bt_cb(skb)->tx_seq != tx_seq)
+ if (bt_cb(skb)->control.txseq != tx_seq)
break;

skb = skb_dequeue(&chan->srej_q);
- control = __set_ctrl_sar(chan, bt_cb(skb)->sar);
+ control = __set_ctrl_sar(chan, bt_cb(skb)->control.sar);
err = l2cap_reassemble_sdu(chan, skb, control);

if (err < 0) {
@@ -4319,8 +4319,8 @@ expected:
chan->expected_tx_seq = __next_seq(chan, chan->expected_tx_seq);

if (test_bit(CONN_SREJ_SENT, &chan->conn_state)) {
- bt_cb(skb)->tx_seq = tx_seq;
- bt_cb(skb)->sar = sar;
+ bt_cb(skb)->control.txseq = tx_seq;
+ bt_cb(skb)->control.sar = sar;
__skb_queue_tail(&chan->srej_q, skb);
return 0;
}
--
1.7.10

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

2012-04-25 23:36:12