* [PATCH 2/6] Bluetooth: Fix freeing uninitialized delayed works
2012-09-06 12:05 [PATCH 1/6] Bluetooth: Fix deadlock when closing socket Andrei Emeltchenko
@ 2012-09-06 12:05 ` Andrei Emeltchenko
2012-09-06 17:03 ` Mat Martineau
2012-09-08 20:28 ` Gustavo Padovan
2012-09-06 12:05 ` [PATCH 3/6] Bluetooth: trivial: Make hci_chan_del return void Andrei Emeltchenko
` (4 subsequent siblings)
5 siblings, 2 replies; 21+ messages in thread
From: Andrei Emeltchenko @ 2012-09-06 12:05 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
When releasing L2CAP socket which is in BT_CONFIG state l2cap_chan_close
invokes l2cap_send_disconn_req which cancel delayed works which are only
set in BT_CONNECTED state with l2cap_ertm_init. Add state check before
cancelling those works.
...
[ 9668.574372] [21085] l2cap_sock_release: sock cd065200, sk f073e800
[ 9668.574399] [21085] l2cap_sock_shutdown: sock cd065200, sk f073e800
[ 9668.574411] [21085] l2cap_chan_close: chan f073ec00 state BT_CONFIG sk f073e800
[ 9668.574421] [21085] l2cap_send_disconn_req: chan f073ec00 conn ecc16600
[ 9668.574441] INFO: trying to register non-static key.
[ 9668.574443] the code is fine but needs lockdep annotation.
[ 9668.574446] turning off the locking correctness validator.
[ 9668.574450] Pid: 21085, comm: obex-client Tainted: G O 3.5.0+ #57
[ 9668.574452] Call Trace:
[ 9668.574463] [<c10a64b3>] __lock_acquire+0x12e3/0x1700
[ 9668.574468] [<c10a44fb>] ? trace_hardirqs_on+0xb/0x10
[ 9668.574476] [<c15e4f60>] ? printk+0x4d/0x4f
[ 9668.574479] [<c10a6e38>] lock_acquire+0x88/0x130
[ 9668.574487] [<c1059740>] ? try_to_del_timer_sync+0x60/0x60
[ 9668.574491] [<c1059790>] del_timer_sync+0x50/0xc0
[ 9668.574495] [<c1059740>] ? try_to_del_timer_sync+0x60/0x60
[ 9668.574515] [<f8aa1c23>] l2cap_send_disconn_req+0xe3/0x160 [bluetooth]
...
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
net/bluetooth/l2cap_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 8e4b57b..b47c325 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1089,7 +1089,7 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
if (!conn)
return;
- if (chan->mode == L2CAP_MODE_ERTM) {
+ if (chan->mode == L2CAP_MODE_ERTM && chan->state == BT_CONNECTED) {
__clear_retrans_timer(chan);
__clear_monitor_timer(chan);
__clear_ack_timer(chan);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 2/6] Bluetooth: Fix freeing uninitialized delayed works
2012-09-06 12:05 ` [PATCH 2/6] Bluetooth: Fix freeing uninitialized delayed works Andrei Emeltchenko
@ 2012-09-06 17:03 ` Mat Martineau
2012-09-08 20:28 ` Gustavo Padovan
1 sibling, 0 replies; 21+ messages in thread
From: Mat Martineau @ 2012-09-06 17:03 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
Andrei -
On Thu, 6 Sep 2012, Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> When releasing L2CAP socket which is in BT_CONFIG state l2cap_chan_close
> invokes l2cap_send_disconn_req which cancel delayed works which are only
> set in BT_CONNECTED state with l2cap_ertm_init. Add state check before
> cancelling those works.
>
> ...
> [ 9668.574372] [21085] l2cap_sock_release: sock cd065200, sk f073e800
> [ 9668.574399] [21085] l2cap_sock_shutdown: sock cd065200, sk f073e800
> [ 9668.574411] [21085] l2cap_chan_close: chan f073ec00 state BT_CONFIG sk f073e800
> [ 9668.574421] [21085] l2cap_send_disconn_req: chan f073ec00 conn ecc16600
> [ 9668.574441] INFO: trying to register non-static key.
> [ 9668.574443] the code is fine but needs lockdep annotation.
> [ 9668.574446] turning off the locking correctness validator.
> [ 9668.574450] Pid: 21085, comm: obex-client Tainted: G O 3.5.0+ #57
> [ 9668.574452] Call Trace:
> [ 9668.574463] [<c10a64b3>] __lock_acquire+0x12e3/0x1700
> [ 9668.574468] [<c10a44fb>] ? trace_hardirqs_on+0xb/0x10
> [ 9668.574476] [<c15e4f60>] ? printk+0x4d/0x4f
> [ 9668.574479] [<c10a6e38>] lock_acquire+0x88/0x130
> [ 9668.574487] [<c1059740>] ? try_to_del_timer_sync+0x60/0x60
> [ 9668.574491] [<c1059790>] del_timer_sync+0x50/0xc0
> [ 9668.574495] [<c1059740>] ? try_to_del_timer_sync+0x60/0x60
> [ 9668.574515] [<f8aa1c23>] l2cap_send_disconn_req+0xe3/0x160 [bluetooth]
> ...
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
> net/bluetooth/l2cap_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 8e4b57b..b47c325 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1089,7 +1089,7 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
> if (!conn)
> return;
>
> - if (chan->mode == L2CAP_MODE_ERTM) {
> + if (chan->mode == L2CAP_MODE_ERTM && chan->state == BT_CONNECTED) {
> __clear_retrans_timer(chan);
> __clear_monitor_timer(chan);
> __clear_ack_timer(chan);
> --
> 1.7.9.5
Looks good to me.
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/6] Bluetooth: Fix freeing uninitialized delayed works
2012-09-06 12:05 ` [PATCH 2/6] Bluetooth: Fix freeing uninitialized delayed works Andrei Emeltchenko
2012-09-06 17:03 ` Mat Martineau
@ 2012-09-08 20:28 ` Gustavo Padovan
1 sibling, 0 replies; 21+ messages in thread
From: Gustavo Padovan @ 2012-09-08 20:28 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
Hi Andrei,
* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-09-06 15:05:42 +0300]:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> When releasing L2CAP socket which is in BT_CONFIG state l2cap_chan_close
> invokes l2cap_send_disconn_req which cancel delayed works which are only
> set in BT_CONNECTED state with l2cap_ertm_init. Add state check before
> cancelling those works.
>
> ...
> [ 9668.574372] [21085] l2cap_sock_release: sock cd065200, sk f073e800
> [ 9668.574399] [21085] l2cap_sock_shutdown: sock cd065200, sk f073e800
> [ 9668.574411] [21085] l2cap_chan_close: chan f073ec00 state BT_CONFIG sk f073e800
> [ 9668.574421] [21085] l2cap_send_disconn_req: chan f073ec00 conn ecc16600
> [ 9668.574441] INFO: trying to register non-static key.
> [ 9668.574443] the code is fine but needs lockdep annotation.
> [ 9668.574446] turning off the locking correctness validator.
> [ 9668.574450] Pid: 21085, comm: obex-client Tainted: G O 3.5.0+ #57
> [ 9668.574452] Call Trace:
> [ 9668.574463] [<c10a64b3>] __lock_acquire+0x12e3/0x1700
> [ 9668.574468] [<c10a44fb>] ? trace_hardirqs_on+0xb/0x10
> [ 9668.574476] [<c15e4f60>] ? printk+0x4d/0x4f
> [ 9668.574479] [<c10a6e38>] lock_acquire+0x88/0x130
> [ 9668.574487] [<c1059740>] ? try_to_del_timer_sync+0x60/0x60
> [ 9668.574491] [<c1059790>] del_timer_sync+0x50/0xc0
> [ 9668.574495] [<c1059740>] ? try_to_del_timer_sync+0x60/0x60
> [ 9668.574515] [<f8aa1c23>] l2cap_send_disconn_req+0xe3/0x160 [bluetooth]
> ...
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
> net/bluetooth/l2cap_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
patch has been applied to bluetooth.git. Thanks.
Gustavo
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/6] Bluetooth: trivial: Make hci_chan_del return void
2012-09-06 12:05 [PATCH 1/6] Bluetooth: Fix deadlock when closing socket Andrei Emeltchenko
2012-09-06 12:05 ` [PATCH 2/6] Bluetooth: Fix freeing uninitialized delayed works Andrei Emeltchenko
@ 2012-09-06 12:05 ` Andrei Emeltchenko
2012-09-08 20:33 ` Gustavo Padovan
2012-09-06 12:05 ` [PATCH 4/6] Bluetooth: trivial: Remove empty line Andrei Emeltchenko
` (3 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Andrei Emeltchenko @ 2012-09-06 12:05 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Return code is not needed in hci_chan_del
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
include/net/bluetooth/hci_core.h | 2 +-
net/bluetooth/hci_conn.c | 4 +---
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index fa807a3..4704ca4 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -552,7 +552,7 @@ void hci_conn_hash_flush(struct hci_dev *hdev);
void hci_conn_check_pending(struct hci_dev *hdev);
struct hci_chan *hci_chan_create(struct hci_conn *conn);
-int hci_chan_del(struct hci_chan *chan);
+void hci_chan_del(struct hci_chan *chan);
void hci_chan_list_flush(struct hci_conn *conn);
struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 4dc8227..c809063 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -935,7 +935,7 @@ struct hci_chan *hci_chan_create(struct hci_conn *conn)
return chan;
}
-int hci_chan_del(struct hci_chan *chan)
+void hci_chan_del(struct hci_chan *chan)
{
struct hci_conn *conn = chan->conn;
struct hci_dev *hdev = conn->hdev;
@@ -948,8 +948,6 @@ int hci_chan_del(struct hci_chan *chan)
skb_queue_purge(&chan->data_q);
kfree(chan);
-
- return 0;
}
void hci_chan_list_flush(struct hci_conn *conn)
--
1.7.9.5
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH 4/6] Bluetooth: trivial: Remove empty line
2012-09-06 12:05 [PATCH 1/6] Bluetooth: Fix deadlock when closing socket Andrei Emeltchenko
2012-09-06 12:05 ` [PATCH 2/6] Bluetooth: Fix freeing uninitialized delayed works Andrei Emeltchenko
2012-09-06 12:05 ` [PATCH 3/6] Bluetooth: trivial: Make hci_chan_del return void Andrei Emeltchenko
@ 2012-09-06 12:05 ` Andrei Emeltchenko
2012-09-08 20:34 ` Gustavo Padovan
2012-09-06 12:05 ` [PATCH 5/6] Bluetooth: debug: Print refcnt for hci_dev Andrei Emeltchenko
` (2 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Andrei Emeltchenko @ 2012-09-06 12:05 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
net/bluetooth/hci_core.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 8dbbc01..f305e44 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -268,7 +268,6 @@ static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
BT_ERR("Unknown device type %d", hdev->dev_type);
break;
}
-
}
static void hci_le_init_req(struct hci_dev *hdev, unsigned long opt)
--
1.7.9.5
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 4/6] Bluetooth: trivial: Remove empty line
2012-09-06 12:05 ` [PATCH 4/6] Bluetooth: trivial: Remove empty line Andrei Emeltchenko
@ 2012-09-08 20:34 ` Gustavo Padovan
0 siblings, 0 replies; 21+ messages in thread
From: Gustavo Padovan @ 2012-09-08 20:34 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
Hi Andrei,
* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-09-06 15:05:44 +0300]:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
> net/bluetooth/hci_core.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 8dbbc01..f305e44 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -268,7 +268,6 @@ static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
> BT_ERR("Unknown device type %d", hdev->dev_type);
> break;
> }
> -
> }
>
> static void hci_le_init_req(struct hci_dev *hdev, unsigned long opt)
Patch has been applied to bluetooth-next. Thanks.
Gustavo
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 5/6] Bluetooth: debug: Print refcnt for hci_dev
2012-09-06 12:05 [PATCH 1/6] Bluetooth: Fix deadlock when closing socket Andrei Emeltchenko
` (2 preceding siblings ...)
2012-09-06 12:05 ` [PATCH 4/6] Bluetooth: trivial: Remove empty line Andrei Emeltchenko
@ 2012-09-06 12:05 ` Andrei Emeltchenko
2012-09-08 21:13 ` Gustavo Padovan
2012-09-06 12:05 ` [PATCH 6/6] Bluetooth: AMP: Add Read Data Block Size to amp_init Andrei Emeltchenko
2012-09-06 17:01 ` [PATCH 1/6] Bluetooth: Fix deadlock when closing socket Mat Martineau
5 siblings, 1 reply; 21+ messages in thread
From: Andrei Emeltchenko @ 2012-09-06 12:05 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
include/net/bluetooth/hci_core.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 4704ca4..6a3337e 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -603,11 +603,17 @@ static inline void hci_conn_put(struct hci_conn *conn)
/* ----- HCI Devices ----- */
static inline void hci_dev_put(struct hci_dev *d)
{
+ BT_DBG("%s orig refcnt %d", d->name,
+ atomic_read(&d->dev.kobj.kref.refcount));
+
put_device(&d->dev);
}
static inline struct hci_dev *hci_dev_hold(struct hci_dev *d)
{
+ BT_DBG("%s orig refcnt %d", d->name,
+ atomic_read(&d->dev.kobj.kref.refcount));
+
get_device(&d->dev);
return d;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 5/6] Bluetooth: debug: Print refcnt for hci_dev
2012-09-06 12:05 ` [PATCH 5/6] Bluetooth: debug: Print refcnt for hci_dev Andrei Emeltchenko
@ 2012-09-08 21:13 ` Gustavo Padovan
0 siblings, 0 replies; 21+ messages in thread
From: Gustavo Padovan @ 2012-09-08 21:13 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
Hi Andrei,
* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-09-06 15:05:45 +0300]:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
> include/net/bluetooth/hci_core.h | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 4704ca4..6a3337e 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -603,11 +603,17 @@ static inline void hci_conn_put(struct hci_conn *conn)
> /* ----- HCI Devices ----- */
> static inline void hci_dev_put(struct hci_dev *d)
> {
> + BT_DBG("%s orig refcnt %d", d->name,
> + atomic_read(&d->dev.kobj.kref.refcount));
> +
> put_device(&d->dev);
> }
>
> static inline struct hci_dev *hci_dev_hold(struct hci_dev *d)
> {
> + BT_DBG("%s orig refcnt %d", d->name,
> + atomic_read(&d->dev.kobj.kref.refcount));
> +
> get_device(&d->dev);
> return d;
> }
Patch has been applied to bluetooth-next. Thanks.
Gustavo
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 6/6] Bluetooth: AMP: Add Read Data Block Size to amp_init
2012-09-06 12:05 [PATCH 1/6] Bluetooth: Fix deadlock when closing socket Andrei Emeltchenko
` (3 preceding siblings ...)
2012-09-06 12:05 ` [PATCH 5/6] Bluetooth: debug: Print refcnt for hci_dev Andrei Emeltchenko
@ 2012-09-06 12:05 ` Andrei Emeltchenko
2012-09-08 21:14 ` Gustavo Padovan
2012-09-06 17:01 ` [PATCH 1/6] Bluetooth: Fix deadlock when closing socket Mat Martineau
5 siblings, 1 reply; 21+ messages in thread
From: Andrei Emeltchenko @ 2012-09-06 12:05 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Add Read Data Block Size HCI cmd to AMP initialization, then it
makes possible to send data.
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
net/bluetooth/hci_core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index f305e44..ab4fca2 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -231,6 +231,9 @@ static void amp_init(struct hci_dev *hdev)
/* Read Local AMP Info */
hci_send_cmd(hdev, HCI_OP_READ_LOCAL_AMP_INFO, 0, NULL);
+
+ /* Read Data Blk size */
+ hci_send_cmd(hdev, HCI_OP_READ_DATA_BLOCK_SIZE, 0, NULL);
}
static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
--
1.7.9.5
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 1/6] Bluetooth: Fix deadlock when closing socket
2012-09-06 12:05 [PATCH 1/6] Bluetooth: Fix deadlock when closing socket Andrei Emeltchenko
` (4 preceding siblings ...)
2012-09-06 12:05 ` [PATCH 6/6] Bluetooth: AMP: Add Read Data Block Size to amp_init Andrei Emeltchenko
@ 2012-09-06 17:01 ` Mat Martineau
2012-09-06 20:10 ` Marcel Holtmann
2012-09-07 14:07 ` [PATCH 1/6] Bluetooth: Fix deadlock when closing socket Andrei Emeltchenko
5 siblings, 2 replies; 21+ messages in thread
From: Mat Martineau @ 2012-09-06 17:01 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth, gustavo
Hi Andrei -
On Thu, 6 Sep 2012, Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> If we have unacked frames when closing bluetooth socket we deadlock
> since conn->chan_lock, chan->lock and socket lock are taken. Remove
> __l2cap_wait_ack completely.
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
I don't think you want to remove this code completely, at least not
without giving some thought to the problem it is solving.
The problem is that programs may have an open socket which they send
some data on, then immediately close. There is no feedback when data
is actually sent over the air, so the socket may end up getting torn
down while there is still data in the HCI tx buffer or some data was
lost and needs to be retransmitted. Waiting for an acknowledgement
confirms that the application's sent data made it to the remote
device.
Without this code, it's difficult to use l2test on a number of
qualification tests. Profiles or applications using ERTM may depend
on the "wait for ack before closing" behavior in order to have a clean
disconnect.
It is not reasonable to deadlock while waiting for unacked packets to
go to 0, so maybe more needs to be done in __l2cap_wait_ack to limit
the wait time.
> ---
> include/net/bluetooth/l2cap.h | 1 -
> net/bluetooth/l2cap_core.c | 32 --------------------------------
> net/bluetooth/l2cap_sock.c | 3 ---
> 3 files changed, 36 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 4e188dc..0330894 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -753,7 +753,6 @@ int l2cap_init_sockets(void);
> void l2cap_cleanup_sockets(void);
>
> void __l2cap_connect_rsp_defer(struct l2cap_chan *chan);
> -int __l2cap_wait_ack(struct sock *sk);
>
> int l2cap_add_psm(struct l2cap_chan *chan, bdaddr_t *src, __le16 psm);
> int l2cap_add_scid(struct l2cap_chan *chan, __u16 scid);
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 3686506..8e4b57b 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1633,38 +1633,6 @@ done:
> return err;
> }
>
> -int __l2cap_wait_ack(struct sock *sk)
> -{
> - struct l2cap_chan *chan = l2cap_pi(sk)->chan;
> - DECLARE_WAITQUEUE(wait, current);
> - int err = 0;
> - int timeo = HZ/5;
> -
> - add_wait_queue(sk_sleep(sk), &wait);
> - set_current_state(TASK_INTERRUPTIBLE);
> - while (chan->unacked_frames > 0 && chan->conn) {
> - if (!timeo)
> - timeo = HZ/5;
> -
> - if (signal_pending(current)) {
> - err = sock_intr_errno(timeo);
> - break;
> - }
> -
> - release_sock(sk);
> - timeo = schedule_timeout(timeo);
> - lock_sock(sk);
> - set_current_state(TASK_INTERRUPTIBLE);
> -
> - err = sock_error(sk);
> - if (err)
> - break;
> - }
> - set_current_state(TASK_RUNNING);
> - remove_wait_queue(sk_sleep(sk), &wait);
> - return err;
> -}
> -
> static void l2cap_monitor_timeout(struct work_struct *work)
> {
> struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 997e0cb..cc26b1f 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -866,9 +866,6 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
> lock_sock(sk);
>
> if (!sk->sk_shutdown) {
> - if (chan->mode == L2CAP_MODE_ERTM)
> - err = __l2cap_wait_ack(sk);
> -
> sk->sk_shutdown = SHUTDOWN_MASK;
>
> release_sock(sk);
> --
> 1.7.9.5
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] 21+ messages in thread* Re: [PATCH 1/6] Bluetooth: Fix deadlock when closing socket
2012-09-06 17:01 ` [PATCH 1/6] Bluetooth: Fix deadlock when closing socket Mat Martineau
@ 2012-09-06 20:10 ` Marcel Holtmann
2012-09-07 14:08 ` Andrei Emeltchenko
2012-09-07 14:07 ` [PATCH 1/6] Bluetooth: Fix deadlock when closing socket Andrei Emeltchenko
1 sibling, 1 reply; 21+ messages in thread
From: Marcel Holtmann @ 2012-09-06 20:10 UTC (permalink / raw)
To: Mat Martineau; +Cc: Andrei Emeltchenko, linux-bluetooth, gustavo
Hi Mat,
> > If we have unacked frames when closing bluetooth socket we deadlock
> > since conn->chan_lock, chan->lock and socket lock are taken. Remove
> > __l2cap_wait_ack completely.
> >
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> I don't think you want to remove this code completely, at least not
> without giving some thought to the problem it is solving.
>
> The problem is that programs may have an open socket which they send
> some data on, then immediately close. There is no feedback when data
> is actually sent over the air, so the socket may end up getting torn
> down while there is still data in the HCI tx buffer or some data was
> lost and needs to be retransmitted. Waiting for an acknowledgement
> confirms that the application's sent data made it to the remote
> device.
>
> Without this code, it's difficult to use l2test on a number of
> qualification tests. Profiles or applications using ERTM may depend
> on the "wait for ack before closing" behavior in order to have a clean
> disconnect.
isn't that what we have SO_LINGER for?
Regards
Marcel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/6] Bluetooth: Fix deadlock when closing socket
2012-09-06 20:10 ` Marcel Holtmann
@ 2012-09-07 14:08 ` Andrei Emeltchenko
2012-09-07 17:00 ` Mat Martineau
0 siblings, 1 reply; 21+ messages in thread
From: Andrei Emeltchenko @ 2012-09-07 14:08 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Mat Martineau, linux-bluetooth, gustavo
Hi Marcel,
On Thu, Sep 06, 2012 at 01:10:51PM -0700, Marcel Holtmann wrote:
> Hi Mat,
>
> > > If we have unacked frames when closing bluetooth socket we deadlock
> > > since conn->chan_lock, chan->lock and socket lock are taken. Remove
> > > __l2cap_wait_ack completely.
> > >
> > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > I don't think you want to remove this code completely, at least not
> > without giving some thought to the problem it is solving.
> >
> > The problem is that programs may have an open socket which they send
> > some data on, then immediately close. There is no feedback when data
> > is actually sent over the air, so the socket may end up getting torn
> > down while there is still data in the HCI tx buffer or some data was
> > lost and needs to be retransmitted. Waiting for an acknowledgement
> > confirms that the application's sent data made it to the remote
> > device.
> >
> > Without this code, it's difficult to use l2test on a number of
> > qualification tests. Profiles or applications using ERTM may depend
> > on the "wait for ack before closing" behavior in order to have a clean
> > disconnect.
>
> isn't that what we have SO_LINGER for?
Looking at the code I suspect that SO_LINGER is not working. Maybe we need
to merge linger code and wait_ack stuff.
Best regards
Andrei Emeltchenko
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/6] Bluetooth: Fix deadlock when closing socket
2012-09-07 14:08 ` Andrei Emeltchenko
@ 2012-09-07 17:00 ` Mat Martineau
2012-09-08 21:05 ` Gustavo Padovan
2012-09-10 8:23 ` [RFCv0] Bluetooth: Fix SO_LINGER in L2CAP Andrei Emeltchenko
0 siblings, 2 replies; 21+ messages in thread
From: Mat Martineau @ 2012-09-07 17:00 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: Marcel Holtmann, linux-bluetooth, gustavo
Andrei -
On Fri, 7 Sep 2012, Andrei Emeltchenko wrote:
> Hi Marcel,
>
> On Thu, Sep 06, 2012 at 01:10:51PM -0700, Marcel Holtmann wrote:
>> Hi Mat,
>>
>>>> If we have unacked frames when closing bluetooth socket we deadlock
>>>> since conn->chan_lock, chan->lock and socket lock are taken. Remove
>>>> __l2cap_wait_ack completely.
>>>>
>>>> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>>>
>>> I don't think you want to remove this code completely, at least not
>>> without giving some thought to the problem it is solving.
>>>
>>> The problem is that programs may have an open socket which they send
>>> some data on, then immediately close. There is no feedback when data
>>> is actually sent over the air, so the socket may end up getting torn
>>> down while there is still data in the HCI tx buffer or some data was
>>> lost and needs to be retransmitted. Waiting for an acknowledgement
>>> confirms that the application's sent data made it to the remote
>>> device.
>>>
>>> Without this code, it's difficult to use l2test on a number of
>>> qualification tests. Profiles or applications using ERTM may depend
>>> on the "wait for ack before closing" behavior in order to have a clean
>>> disconnect.
>>
>> isn't that what we have SO_LINGER for?
>
> Looking at the code I suspect that SO_LINGER is not working. Maybe we need
> to merge linger code and wait_ack stuff.
It does look like a bug that the "wait_ack" behavior happens even
without SO_LINGER.
In order to do SO_LINGER right, it would be better to check chan->tx_q
instead of chan->unacked_frames to makes sure all data is sent and
acked. chan->unacked_frames only tells you how many sent frames are
unacked and does not take in to account queued data that hasn't been
sent yet.
--
Mat Martineau
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/6] Bluetooth: Fix deadlock when closing socket
2012-09-07 17:00 ` Mat Martineau
@ 2012-09-08 21:05 ` Gustavo Padovan
2012-09-10 8:23 ` [RFCv0] Bluetooth: Fix SO_LINGER in L2CAP Andrei Emeltchenko
1 sibling, 0 replies; 21+ messages in thread
From: Gustavo Padovan @ 2012-09-08 21:05 UTC (permalink / raw)
To: Mat Martineau; +Cc: Andrei Emeltchenko, Marcel Holtmann, linux-bluetooth
* Mat Martineau <mathewm@codeaurora.org> [2012-09-07 10:00:40 -0700]:
>
> Andrei -
>
> On Fri, 7 Sep 2012, Andrei Emeltchenko wrote:
>
> >Hi Marcel,
> >
> >On Thu, Sep 06, 2012 at 01:10:51PM -0700, Marcel Holtmann wrote:
> >>Hi Mat,
> >>
> >>>>If we have unacked frames when closing bluetooth socket we deadlock
> >>>>since conn->chan_lock, chan->lock and socket lock are taken. Remove
> >>>>__l2cap_wait_ack completely.
> >>>>
> >>>>Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >>>
> >>>I don't think you want to remove this code completely, at least not
> >>>without giving some thought to the problem it is solving.
> >>>
> >>>The problem is that programs may have an open socket which they send
> >>>some data on, then immediately close. There is no feedback when data
> >>>is actually sent over the air, so the socket may end up getting torn
> >>>down while there is still data in the HCI tx buffer or some data was
> >>>lost and needs to be retransmitted. Waiting for an acknowledgement
> >>>confirms that the application's sent data made it to the remote
> >>>device.
> >>>
> >>>Without this code, it's difficult to use l2test on a number of
> >>>qualification tests. Profiles or applications using ERTM may depend
> >>>on the "wait for ack before closing" behavior in order to have a clean
> >>>disconnect.
> >>
> >>isn't that what we have SO_LINGER for?
> >
> >Looking at the code I suspect that SO_LINGER is not working. Maybe we need
> >to merge linger code and wait_ack stuff.
>
> It does look like a bug that the "wait_ack" behavior happens even
> without SO_LINGER.
>
> In order to do SO_LINGER right, it would be better to check
> chan->tx_q instead of chan->unacked_frames to makes sure all data is
> sent and acked. chan->unacked_frames only tells you how many sent
> frames are unacked and does not take in to account queued data that
> hasn't been sent yet.
Not sure if we need SO_LINGER here, in TCP shutdown() per default waits for
the remote side to close the connection, we basically doing something similar
here. IMHO we just fixes this to avoid deadlock and stay with this code.
Gustavo
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFCv0] Bluetooth: Fix SO_LINGER in L2CAP
2012-09-07 17:00 ` Mat Martineau
2012-09-08 21:05 ` Gustavo Padovan
@ 2012-09-10 8:23 ` Andrei Emeltchenko
2012-09-10 10:43 ` Anderson Lizardo
1 sibling, 1 reply; 21+ messages in thread
From: Andrei Emeltchenko @ 2012-09-10 8:23 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
include/net/bluetooth/l2cap.h | 2 +-
net/bluetooth/l2cap_core.c | 35 ++++++++++++++++-------------------
net/bluetooth/l2cap_sock.c | 9 +++------
3 files changed, 20 insertions(+), 26 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 8a726ff..72ffe53 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -757,7 +757,7 @@ int l2cap_init_sockets(void);
void l2cap_cleanup_sockets(void);
void __l2cap_connect_rsp_defer(struct l2cap_chan *chan);
-int __l2cap_wait_ack(struct sock *sk);
+int __l2cap_wait_ack(struct sock *sk, unsigned long timeo);
int l2cap_add_psm(struct l2cap_chan *chan, bdaddr_t *src, __le16 psm);
int l2cap_add_scid(struct l2cap_chan *chan, __u16 scid);
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b1914e6..0502042 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1696,35 +1696,32 @@ done:
return err;
}
-int __l2cap_wait_ack(struct sock *sk)
+int __l2cap_wait_ack(struct sock *sk, unsigned long timeo)
{
struct l2cap_chan *chan = l2cap_pi(sk)->chan;
- DECLARE_WAITQUEUE(wait, current);
int err = 0;
- int timeo = HZ/5;
- add_wait_queue(sk_sleep(sk), &wait);
- set_current_state(TASK_INTERRUPTIBLE);
- while (chan->unacked_frames > 0 && chan->conn) {
- if (!timeo)
- timeo = HZ/5;
+ if (unlikely(!skb_queue_empty(&chan->tx_q))) {
+ DECLARE_WAITQUEUE(wait, current);
+
+ add_wait_queue(sk_sleep(sk), &wait);
+ set_current_state(TASK_INTERRUPTIBLE);
if (signal_pending(current)) {
err = sock_intr_errno(timeo);
- break;
- }
+ } else {
+ release_sock(sk);
+ timeo = schedule_timeout(timeo);
+ lock_sock(sk);
+ set_current_state(TASK_INTERRUPTIBLE);
- release_sock(sk);
- timeo = schedule_timeout(timeo);
- lock_sock(sk);
- set_current_state(TASK_INTERRUPTIBLE);
+ err = sock_error(sk);
+ }
- err = sock_error(sk);
- if (err)
- break;
+ set_current_state(TASK_RUNNING);
+ remove_wait_queue(sk_sleep(sk), &wait);
}
- set_current_state(TASK_RUNNING);
- remove_wait_queue(sk_sleep(sk), &wait);
+
return err;
}
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index f9cdc02..bb3515f 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -866,18 +866,15 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
lock_sock(sk);
if (!sk->sk_shutdown) {
- if (chan->mode == L2CAP_MODE_ERTM)
- err = __l2cap_wait_ack(sk);
+ if (chan->mode == L2CAP_MODE_ERTM &&
+ sock_flag(sk, SOCK_LINGER && sk->sk_lingertime))
+ err = __l2cap_wait_ack(sk, sk->sk_lingertime);
sk->sk_shutdown = SHUTDOWN_MASK;
release_sock(sk);
l2cap_chan_close(chan, 0);
lock_sock(sk);
-
- if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime)
- err = bt_sock_wait_state(sk, BT_CLOSED,
- sk->sk_lingertime);
}
if (!err && sk->sk_err)
--
1.7.9.5
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFCv0] Bluetooth: Fix SO_LINGER in L2CAP
2012-09-10 8:23 ` [RFCv0] Bluetooth: Fix SO_LINGER in L2CAP Andrei Emeltchenko
@ 2012-09-10 10:43 ` Anderson Lizardo
2012-09-10 10:55 ` Andrei Emeltchenko
0 siblings, 1 reply; 21+ messages in thread
From: Anderson Lizardo @ 2012-09-10 10:43 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
Hi Andrei,
On Mon, Sep 10, 2012 at 4:23 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Could you explain on the commit message what is broken on the current
code, so we can review the semantics of your changes?
Is it any crash, wrong behavior etc.
Best Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFCv0] Bluetooth: Fix SO_LINGER in L2CAP
2012-09-10 10:43 ` Anderson Lizardo
@ 2012-09-10 10:55 ` Andrei Emeltchenko
0 siblings, 0 replies; 21+ messages in thread
From: Andrei Emeltchenko @ 2012-09-10 10:55 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
Hi Anderson,
On Mon, Sep 10, 2012 at 06:43:53AM -0400, Anderson Lizardo wrote:
> Hi Andrei,
>
> On Mon, Sep 10, 2012 at 4:23 AM, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Could you explain on the commit message what is broken on the current
> code, so we can review the semantics of your changes?
If we have unacked frames when closing bluetooth socket we deadlock
since conn->chan_lock, chan->lock and socket lock are taken.
Also see Mat comments.
Best regards
Andrei Emeltchenko
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/6] Bluetooth: Fix deadlock when closing socket
2012-09-06 17:01 ` [PATCH 1/6] Bluetooth: Fix deadlock when closing socket Mat Martineau
2012-09-06 20:10 ` Marcel Holtmann
@ 2012-09-07 14:07 ` Andrei Emeltchenko
1 sibling, 0 replies; 21+ messages in thread
From: Andrei Emeltchenko @ 2012-09-07 14:07 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo
Hi Mat,
On Thu, Sep 06, 2012 at 10:01:52AM -0700, Mat Martineau wrote:
>
> Hi Andrei -
>
> On Thu, 6 Sep 2012, Andrei Emeltchenko wrote:
>
> >From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> >If we have unacked frames when closing bluetooth socket we deadlock
> >since conn->chan_lock, chan->lock and socket lock are taken. Remove
> >__l2cap_wait_ack completely.
> >
> >Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> I don't think you want to remove this code completely, at least not
> without giving some thought to the problem it is solving.
>
> The problem is that programs may have an open socket which they send
> some data on, then immediately close. There is no feedback when
> data is actually sent over the air, so the socket may end up getting
> torn down while there is still data in the HCI tx buffer or some
> data was lost and needs to be retransmitted. Waiting for an
> acknowledgement confirms that the application's sent data made it to
> the remote device.
>
> Without this code, it's difficult to use l2test on a number of
> qualification tests. Profiles or applications using ERTM may depend
> on the "wait for ack before closing" behavior in order to have a
> clean disconnect.
>
> It is not reasonable to deadlock while waiting for unacked packets
> to go to 0, so maybe more needs to be done in __l2cap_wait_ack to
> limit the wait time.
Have you seen my previous RFC concerning this? It has 3 tries and then
exits from the loop.
Best regards
Andrei Emeltchenko
^ permalink raw reply [flat|nested] 21+ messages in thread