* [PATCH 1/2] Bluetooth: Fix L2CAP ERTM packet queue corruption @ 2012-04-25 8:29 Manoj 2012-04-25 8:29 ` [PATCH 2/2] Bluetooth: Fix retransmission of ERTM packets Manoj 2012-04-25 8:46 ` [PATCH 1/2] Bluetooth: Fix L2CAP ERTM packet queue corruption Andrei Emeltchenko 0 siblings, 2 replies; 10+ messages in thread From: Manoj @ 2012-04-25 8:29 UTC (permalink / raw) To: linux-bluetooth; +Cc: Anurag Gupta, Manoj 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 <manojkr.sharma@stericsson.com> --- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] Bluetooth: Fix retransmission of ERTM packets 2012-04-25 8:29 [PATCH 1/2] Bluetooth: Fix L2CAP ERTM packet queue corruption Manoj @ 2012-04-25 8:29 ` Manoj 2012-04-25 15:33 ` Mat Martineau 2012-04-25 8:46 ` [PATCH 1/2] Bluetooth: Fix L2CAP ERTM packet queue corruption Andrei Emeltchenko 1 sibling, 1 reply; 10+ messages in thread From: Manoj @ 2012-04-25 8:29 UTC (permalink / raw) To: linux-bluetooth; +Cc: Anurag Gupta, Manoj 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 <manojkr.sharma@stericsson.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 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Fix retransmission of ERTM packets 2012-04-25 8:29 ` [PATCH 2/2] Bluetooth: Fix retransmission of ERTM packets Manoj @ 2012-04-25 15:33 ` Mat Martineau 2012-04-25 17:29 ` Manoj Sharma 2013-01-11 8:32 ` Obexd OPP filesend fails with Windows7 stack Syam Sidhardhan 0 siblings, 2 replies; 10+ messages in thread From: Mat Martineau @ 2012-04-25 15:33 UTC (permalink / raw) To: Manoj; +Cc: linux-bluetooth, Anurag Gupta 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 <manojkr.sharma@stericsson.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 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Fix retransmission of ERTM packets 2012-04-25 15:33 ` Mat Martineau @ 2012-04-25 17:29 ` Manoj Sharma 2013-01-11 8:32 ` Obexd OPP filesend fails with Windows7 stack Syam Sidhardhan 1 sibling, 0 replies; 10+ messages in thread From: Manoj Sharma @ 2012-04-25 17:29 UTC (permalink / raw) To: Mat Martineau; +Cc: linux-bluetooth, Anurag Gupta Hi Mat, On 4/25/12, Mat Martineau <mathewm@codeaurora.org> 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 <manojkr.sharma@stericsson.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 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Regards, -- Manoj Sharma Employee of ST Ericsson ^ permalink raw reply [flat|nested] 10+ messages in thread
* Obexd OPP filesend fails with Windows7 stack 2012-04-25 15:33 ` Mat Martineau 2012-04-25 17:29 ` Manoj Sharma @ 2013-01-11 8:32 ` Syam Sidhardhan 2013-01-13 15:42 ` Luiz Augusto von Dentz 1 sibling, 1 reply; 10+ messages in thread From: Syam Sidhardhan @ 2013-01-11 8:32 UTC (permalink / raw) To: linux-bluetooth Hi, We are using the obexd 0.48 version and testing the obexd. During testing OPP file send, we found one interoperability issue with the Windows7 PC stack and some commercialized devices available in the market. The issue is: OBEXD file transfer is getting failed once after the complete file content got transfferd. This is because of, we do a direct RFCOMM disconection rather than doing a proper OBEX disconection before. Does someone can help me a with a fix for this? hcidump logs: < ACL data: handle 11 flags 0x00 dlen 15 L2CAP(d): cid 0x0041 len 11 [psm 3] RFCOMM(d): UIH: cr 1 dlci 2 pf 0 ilen 7 fcs 0x9a OBEX: Connect cmd(f): len 7 version 1.0 flags 0 mtu 4096 > HCI Event: Number of Completed Packets (0x13) plen 5 handle 11 packets 2 > ACL data: handle 11 flags 0x02 dlen 9 L2CAP(d): cid 0x0041 len 5 [psm 3] RFCOMM(d): UIH: cr 0 dlci 2 pf 1 ilen 0 fcs 0x5c credits 26 > ACL data: handle 11 flags 0x02 dlen 16 L2CAP(d): cid 0x0041 len 12 [psm 3] RFCOMM(d): UIH: cr 0 dlci 2 pf 1 ilen 7 fcs 0x5c credits 0 OBEX: Connect rsp(f): status 200 len 7 version 1.0 flags 0 mtu 4096 Status 200 = Success < ACL data: handle 11 flags 0x00 dlen 65 L2CAP(d): cid 0x0041 len 61 [psm 3] RFCOMM(d): UIH: cr 1 dlci 2 pf 0 ilen 57 fcs 0x9a OBEX: Put cmd(c): len 57 Name (0x01) = Unicode length 28 0000: 00 74 00 65 00 73 00 74 00 2d 00 74 00 65 00 78 .t.e.s.t.-.t.e.x 0010: 00 74 00 2e 00 74 00 78 00 74 00 00 .t...t.x.t.. Length (0xc3) = 15 Body (0x48) = Sequence length 15 0000: 54 65 73 74 20 44 6f 63 75 6d 65 6e 74 0d 0a Test Document.. > ACL data: handle 11 flags 0x02 dlen 12 L2CAP(d): cid 0x0041 len 8 [psm 3] RFCOMM(d): UIH: cr 0 dlci 2 pf 1 ilen 3 fcs 0x5c credits 1 OBEX: Put rsp(f): status 100 len 3 < ACL data: handle 11 flags 0x00 dlen 14 L2CAP(d): cid 0x0041 len 10 [psm 3] RFCOMM(d): UIH: cr 1 dlci 2 pf 0 ilen 6 fcs 0x9a OBEX: Put cmd(f): len 6 (continue) End of Body (0x49) = Sequence length 0 > HCI Event: Number of Completed Packets (0x13) plen 5 handle 11 packets 2 > ACL data: handle 11 flags 0x02 dlen 12 L2CAP(d): cid 0x0041 len 8 [psm 3] RFCOMM(d): UIH: cr 0 dlci 2 pf 1 ilen 3 fcs 0x5c credits 1 OBEX: Put rsp(f): status 200 len 3 < ACL data: handle 11 flags 0x00 dlen 8 L2CAP(d): cid 0x0041 len 4 [psm 3] RFCOMM(s): DISC: cr 1 dlci 2 pf 1 ilen 0 fcs 0xb8 > ACL data: handle 11 flags 0x02 dlen 8 L2CAP(d): cid 0x0041 len 4 [psm 3] RFCOMM(s): UA: cr 1 dlci 2 pf 1 ilen 0 fcs 0x92 < ACL data: handle 11 flags 0x00 dlen 8 L2CAP(d): cid 0x0041 len 4 [psm 3] RFCOMM(s): DISC: cr 1 dlci 0 pf 1 ilen 0 fcs 0xfd < ACL data: handle 11 flags 0x00 dlen 12 L2CAP(s): Disconn req: dcid 0x0041 scid 0x0041 > HCI Event: Number of Completed Packets (0x13) plen 5 handle 11 packets 2 > ACL data: handle 11 flags 0x02 dlen 8 L2CAP(d): cid 0x0041 len 4 [psm 3] RFCOMM(s): UA: cr 1 dlci 0 pf 1 ilen 0 fcs 0xd7 Thanks, Syam. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Obexd OPP filesend fails with Windows7 stack 2013-01-11 8:32 ` Obexd OPP filesend fails with Windows7 stack Syam Sidhardhan @ 2013-01-13 15:42 ` Luiz Augusto von Dentz 2013-01-13 18:35 ` Syam Sidhardhan 0 siblings, 1 reply; 10+ messages in thread From: Luiz Augusto von Dentz @ 2013-01-13 15:42 UTC (permalink / raw) To: Syam Sidhardhan; +Cc: linux-bluetooth@vger.kernel.org Hi Syam, On Fri, Jan 11, 2013 at 10:32 AM, Syam Sidhardhan <s.syam@samsung.com> wrote: > Hi, > > We are using the obexd 0.48 version and testing the obexd. > During testing OPP file send, we found one interoperability > issue with the Windows7 PC stack and some commercialized devices > available in the market. > > The issue is: OBEXD file transfer is getting failed > once after the complete file content got transfferd. This is > because of, we do a direct RFCOMM disconection rather than > doing a proper OBEX disconection before. > > Does someone can help me a with a fix for this? We probably need to send a disconnect command before disconnecting RFCOMM, iirc there was someone else looking into a similar problem with a Nokia phone, the strange part is that the remote stack has acknowledged the transfer complete and in theory we could go ahead and start another one using the same session. -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Obexd OPP filesend fails with Windows7 stack 2013-01-13 15:42 ` Luiz Augusto von Dentz @ 2013-01-13 18:35 ` Syam Sidhardhan 0 siblings, 0 replies; 10+ messages in thread From: Syam Sidhardhan @ 2013-01-13 18:35 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: Syam Sidhardhan, linux-bluetooth@vger.kernel.org Hi Luiz, On Sun, Jan 13, 2013 at 9:12 PM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > Hi Syam, > > On Fri, Jan 11, 2013 at 10:32 AM, Syam Sidhardhan <s.syam@samsung.com> wrote: >> Hi, >> >> We are using the obexd 0.48 version and testing the obexd. >> During testing OPP file send, we found one interoperability >> issue with the Windows7 PC stack and some commercialized devices >> available in the market. >> >> The issue is: OBEXD file transfer is getting failed >> once after the complete file content got transfferd. This is >> because of, we do a direct RFCOMM disconection rather than >> doing a proper OBEX disconection before. >> >> Does someone can help me a with a fix for this? > > We probably need to send a disconnect command before disconnecting > RFCOMM, iirc there was someone else looking into a similar problem > with a Nokia phone, the strange part is that the remote stack has > acknowledged the transfer complete and in theory we could go ahead and > start another one using the same session. > Yes, you are correct. We need to send a OBEX disconnect command before the RFCOMM disconnection. We have tried with the same and its working fine. Regards, Syam ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Fix L2CAP ERTM packet queue corruption 2012-04-25 8:29 [PATCH 1/2] Bluetooth: Fix L2CAP ERTM packet queue corruption Manoj 2012-04-25 8:29 ` [PATCH 2/2] Bluetooth: Fix retransmission of ERTM packets Manoj @ 2012-04-25 8:46 ` Andrei Emeltchenko 2012-04-25 16:05 ` Mat Martineau 1 sibling, 1 reply; 10+ messages in thread From: Andrei Emeltchenko @ 2012-04-25 8:46 UTC (permalink / raw) To: Manoj; +Cc: linux-bluetooth, Anurag Gupta 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 <manojkr.sharma@stericsson.com> > --- > 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Fix L2CAP ERTM packet queue corruption 2012-04-25 8:46 ` [PATCH 1/2] Bluetooth: Fix L2CAP ERTM packet queue corruption Andrei Emeltchenko @ 2012-04-25 16:05 ` Mat Martineau 2012-04-25 17:26 ` Manoj Sharma 0 siblings, 1 reply; 10+ messages in thread From: Mat Martineau @ 2012-04-25 16:05 UTC (permalink / raw) To: Manoj; +Cc: Andrei Emeltchenko, linux-bluetooth, Anurag Gupta 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 <manojkr.sharma@stericsson.com> >> --- >> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Bluetooth: Fix L2CAP ERTM packet queue corruption 2012-04-25 16:05 ` Mat Martineau @ 2012-04-25 17:26 ` Manoj Sharma 0 siblings, 0 replies; 10+ messages in thread From: Manoj Sharma @ 2012-04-25 17:26 UTC (permalink / raw) To: Mat Martineau; +Cc: Andrei Emeltchenko, linux-bluetooth, Anurag Gupta Hi Mat, On 4/25/12, Mat Martineau <mathewm@codeaurora.org> 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 <manojkr.sharma@stericsson.com> >>> --- >>> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Regards, -- Manoj Sharma Employee of ST Ericsson ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-01-13 18:35 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-04-25 8:29 [PATCH 1/2] Bluetooth: Fix L2CAP ERTM packet queue corruption Manoj 2012-04-25 8:29 ` [PATCH 2/2] Bluetooth: Fix retransmission of ERTM packets Manoj 2012-04-25 15:33 ` Mat Martineau 2012-04-25 17:29 ` Manoj Sharma 2013-01-11 8:32 ` Obexd OPP filesend fails with Windows7 stack Syam Sidhardhan 2013-01-13 15:42 ` Luiz Augusto von Dentz 2013-01-13 18:35 ` Syam Sidhardhan 2012-04-25 8:46 ` [PATCH 1/2] Bluetooth: Fix L2CAP ERTM packet queue corruption Andrei Emeltchenko 2012-04-25 16:05 ` Mat Martineau 2012-04-25 17:26 ` Manoj Sharma
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox