While running in L2CAP ERTM mode, sometimes ERTM packet queue
gets corrupted because though method l2cap_ertm_send() is not
thread-safe, it is called simultaneously from multiple
threads.
Signed-off-by: Manoj <[email protected]>
---
net/bluetooth/l2cap_core.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 19807c9..a9319e2 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1713,6 +1713,7 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
if (chan->state != BT_CONNECTED)
return -ENOTCONN;
+ mutex_lock(&chan->conn->chan_lock);
while ((skb = chan->tx_send_head) && (!l2cap_tx_window_full(chan))) {
if (chan->remote_max_tx &&
@@ -1765,7 +1766,7 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
else
chan->tx_send_head = skb_queue_next(&chan->tx_q, skb);
}
-
+ mutex_unlock(&chan->conn->chan_lock);
return nsent;
}
--
1.6.6.1
Hi Mat,
On 4/25/12, Mat Martineau <[email protected]> wrote:
>
> Hi Manoj -
>
> On Wed, 25 Apr 2012, Manoj wrote:
>
>> State CONN_REJ_ACT of connection is not handled properly.
>> ERTM packets should be transmitted only if bit corresponding to
>> CONN_REJ_ACT in conn_state field is set. Opposite was being
>> done, which has been fixed by this patch.
>
> This is incorrect. CONN_REJ_ACT maps to "RejActioned" in the ERTM
> spec. RejActioned means the REJ has already been processed but there
> was an outstanding Poll, and the Final flag should not trigger
> retransmission of iframes. Please review the spec and let us know if
> you still think there is a problem with CONN_REJ_ACT.
>
Thanks for explaining the exact usage of CONN_REJ_ACT. I had
misunderstood it. After your explanation, I completely agree with the
existing implementation. Therefore my patch can be ignored.
>>
>> Signed-off-by: Manoj <[email protected]>
>> ---
>> 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 a9319e2..485d374 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -4324,7 +4324,7 @@ expected:
>> }
>>
>> if (__is_ctrl_final(chan, rx_control)) {
>> - if (!test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
>> + if (test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
>> l2cap_retransmit_frames(chan);
>> }
>>
>> @@ -4366,7 +4366,7 @@ static inline void l2cap_data_channel_rrframe(struct
>> l2cap_chan *chan, u32 rx_co
>> } else if (__is_ctrl_final(chan, rx_control)) {
>> clear_bit(CONN_REMOTE_BUSY, &chan->conn_state);
>>
>> - if (!test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
>> + if (test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
>> l2cap_retransmit_frames(chan);
>>
>> } else {
>> @@ -4394,7 +4394,7 @@ static inline void
>> l2cap_data_channel_rejframe(struct l2cap_chan *chan, u32 rx_c
>> l2cap_drop_acked_frames(chan);
>>
>> if (__is_ctrl_final(chan, rx_control)) {
>> - if (!test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
>> + if (test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
>> l2cap_retransmit_frames(chan);
>> } else {
>> l2cap_retransmit_frames(chan);
>> --
>> 1.6.6.1
>>
>
> Regards,
> --
> Mat Martineau
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
> in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Regards,
--
Manoj Sharma
Employee of ST Ericsson
Hi Mat,
On 4/25/12, Mat Martineau <[email protected]> wrote:
>
> Manoj -
>
> On Wed, 25 Apr 2012, Andrei Emeltchenko wrote:
>
>> Hi Manoj,
>>
>> On Wed, Apr 25, 2012 at 01:59:03PM +0530, Manoj wrote:
>>> While running in L2CAP ERTM mode, sometimes ERTM packet queue
>>> gets corrupted because though method l2cap_ertm_send() is not
>>> thread-safe, it is called simultaneously from multiple
>>> threads.
>>
>> Could you give examples how queue is corrupted?
>>
>> Best regards
>> Andrei Emeltchenko
>
> Around the time that bluetooth-next made the switch to workqueues, I
> saw some issues where the packet sent in l2cap_ertm_send could be
> acked in tasklet context before l2cap_ertm_send finished running.
> This would cause the packet to be removed from the tx_q before
> skb_queue_is_last() was called, which would in turn result in a
> corrupt tx_send_head pointer.
>
> With workqueues and proper locking, this should not be a problem.
> The lock that needs to be held when changing tx_q is acquired using
> l2cap_chan_lock(), *not* the mutex_lock() used in this patch. Callers
> of l2cap_ertm_send() should already hold l2cap_chan_lock(), which will
> provide thread safety. It looks like l2cap_chan_lock() is not held
> when l2cap_ertm_send() is called by l2cap_chan_send().
>
> (Also keep in mind that l2cap_chan_lock() cannot be held when
> l2cap_create_iframe_pdu() is called, because it's possible to block
> while allocating iframe skbs)
>
Yes, I agree with your points about the reasons of corruption. Do you
have any solution/patch for this ready? If no, then I would work on a
proper solution based on your feedback. In the mean time I would
appreciate if you could give any suggestion for the solution, based on
your understanding.
>>>
>>> Signed-off-by: Manoj <[email protected]>
>>> ---
>>> net/bluetooth/l2cap_core.c | 3 ++-
>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>>> index 19807c9..a9319e2 100644
>>> --- a/net/bluetooth/l2cap_core.c
>>> +++ b/net/bluetooth/l2cap_core.c
>>> @@ -1713,6 +1713,7 @@ static int l2cap_ertm_send(struct l2cap_chan
>>> *chan)
>>> if (chan->state != BT_CONNECTED)
>>> return -ENOTCONN;
>>>
>>> + mutex_lock(&chan->conn->chan_lock);
>>> while ((skb = chan->tx_send_head) && (!l2cap_tx_window_full(chan))) {
>>>
>>> if (chan->remote_max_tx &&
>>> @@ -1765,7 +1766,7 @@ static int l2cap_ertm_send(struct l2cap_chan
>>> *chan)
>>> else
>>> chan->tx_send_head = skb_queue_next(&chan->tx_q, skb);
>>> }
>>> -
>>> + mutex_unlock(&chan->conn->chan_lock);
>>> return nsent;
>>> }
>>>
>>> --
>>> 1.6.6.1
>
> Regards,
> --
> Mat Martineau
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
> in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Regards,
--
Manoj Sharma
Employee of ST Ericsson
Manoj -
On Wed, 25 Apr 2012, Andrei Emeltchenko wrote:
> Hi Manoj,
>
> On Wed, Apr 25, 2012 at 01:59:03PM +0530, Manoj wrote:
>> While running in L2CAP ERTM mode, sometimes ERTM packet queue
>> gets corrupted because though method l2cap_ertm_send() is not
>> thread-safe, it is called simultaneously from multiple
>> threads.
>
> Could you give examples how queue is corrupted?
>
> Best regards
> Andrei Emeltchenko
Around the time that bluetooth-next made the switch to workqueues, I
saw some issues where the packet sent in l2cap_ertm_send could be
acked in tasklet context before l2cap_ertm_send finished running.
This would cause the packet to be removed from the tx_q before
skb_queue_is_last() was called, which would in turn result in a
corrupt tx_send_head pointer.
With workqueues and proper locking, this should not be a problem.
The lock that needs to be held when changing tx_q is acquired using
l2cap_chan_lock(), *not* the mutex_lock() used in this patch. Callers
of l2cap_ertm_send() should already hold l2cap_chan_lock(), which will
provide thread safety. It looks like l2cap_chan_lock() is not held
when l2cap_ertm_send() is called by l2cap_chan_send().
(Also keep in mind that l2cap_chan_lock() cannot be held when
l2cap_create_iframe_pdu() is called, because it's possible to block
while allocating iframe skbs)
>>
>> Signed-off-by: Manoj <[email protected]>
>> ---
>> net/bluetooth/l2cap_core.c | 3 ++-
>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index 19807c9..a9319e2 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -1713,6 +1713,7 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
>> if (chan->state != BT_CONNECTED)
>> return -ENOTCONN;
>>
>> + mutex_lock(&chan->conn->chan_lock);
>> while ((skb = chan->tx_send_head) && (!l2cap_tx_window_full(chan))) {
>>
>> if (chan->remote_max_tx &&
>> @@ -1765,7 +1766,7 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
>> else
>> chan->tx_send_head = skb_queue_next(&chan->tx_q, skb);
>> }
>> -
>> + mutex_unlock(&chan->conn->chan_lock);
>> return nsent;
>> }
>>
>> --
>> 1.6.6.1
Regards,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Manoj -
On Wed, 25 Apr 2012, Manoj wrote:
> State CONN_REJ_ACT of connection is not handled properly.
> ERTM packets should be transmitted only if bit corresponding to
> CONN_REJ_ACT in conn_state field is set. Opposite was being
> done, which has been fixed by this patch.
This is incorrect. CONN_REJ_ACT maps to "RejActioned" in the ERTM
spec. RejActioned means the REJ has already been processed but there
was an outstanding Poll, and the Final flag should not trigger
retransmission of iframes. Please review the spec and let us know if
you still think there is a problem with CONN_REJ_ACT.
>
> Signed-off-by: Manoj <[email protected]>
> ---
> 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 a9319e2..485d374 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -4324,7 +4324,7 @@ expected:
> }
>
> if (__is_ctrl_final(chan, rx_control)) {
> - if (!test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
> + if (test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
> l2cap_retransmit_frames(chan);
> }
>
> @@ -4366,7 +4366,7 @@ static inline void l2cap_data_channel_rrframe(struct l2cap_chan *chan, u32 rx_co
> } else if (__is_ctrl_final(chan, rx_control)) {
> clear_bit(CONN_REMOTE_BUSY, &chan->conn_state);
>
> - if (!test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
> + if (test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
> l2cap_retransmit_frames(chan);
>
> } else {
> @@ -4394,7 +4394,7 @@ static inline void l2cap_data_channel_rejframe(struct l2cap_chan *chan, u32 rx_c
> l2cap_drop_acked_frames(chan);
>
> if (__is_ctrl_final(chan, rx_control)) {
> - if (!test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
> + if (test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
> l2cap_retransmit_frames(chan);
> } else {
> l2cap_retransmit_frames(chan);
> --
> 1.6.6.1
>
Regards,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Manoj,
On Wed, Apr 25, 2012 at 01:59:03PM +0530, Manoj wrote:
> While running in L2CAP ERTM mode, sometimes ERTM packet queue
> gets corrupted because though method l2cap_ertm_send() is not
> thread-safe, it is called simultaneously from multiple
> threads.
Could you give examples how queue is corrupted?
Best regards
Andrei Emeltchenko
>
> Signed-off-by: Manoj <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 19807c9..a9319e2 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1713,6 +1713,7 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
> if (chan->state != BT_CONNECTED)
> return -ENOTCONN;
>
> + mutex_lock(&chan->conn->chan_lock);
> while ((skb = chan->tx_send_head) && (!l2cap_tx_window_full(chan))) {
>
> if (chan->remote_max_tx &&
> @@ -1765,7 +1766,7 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
> else
> chan->tx_send_head = skb_queue_next(&chan->tx_q, skb);
> }
> -
> + mutex_unlock(&chan->conn->chan_lock);
> return nsent;
> }
>
> --
> 1.6.6.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
State CONN_REJ_ACT of connection is not handled properly.
ERTM packets should be transmitted only if bit corresponding to
CONN_REJ_ACT in conn_state field is set. Opposite was being
done, which has been fixed by this patch.
Signed-off-by: Manoj <[email protected]>
---
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 a9319e2..485d374 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4324,7 +4324,7 @@ expected:
}
if (__is_ctrl_final(chan, rx_control)) {
- if (!test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
+ if (test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
l2cap_retransmit_frames(chan);
}
@@ -4366,7 +4366,7 @@ static inline void l2cap_data_channel_rrframe(struct l2cap_chan *chan, u32 rx_co
} else if (__is_ctrl_final(chan, rx_control)) {
clear_bit(CONN_REMOTE_BUSY, &chan->conn_state);
- if (!test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
+ if (test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
l2cap_retransmit_frames(chan);
} else {
@@ -4394,7 +4394,7 @@ static inline void l2cap_data_channel_rejframe(struct l2cap_chan *chan, u32 rx_c
l2cap_drop_acked_frames(chan);
if (__is_ctrl_final(chan, rx_control)) {
- if (!test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
+ if (test_and_clear_bit(CONN_REJ_ACT, &chan->conn_state))
l2cap_retransmit_frames(chan);
} else {
l2cap_retransmit_frames(chan);
--
1.6.6.1