From: Andrei Emeltchenko <[email protected]>
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 <[email protected]>
---
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
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
> <[email protected]> wrote:
> > From: Andrei Emeltchenko <[email protected]>
>
> 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
Hi Andrei,
On Mon, Sep 10, 2012 at 4:23 AM, Andrei Emeltchenko
<[email protected]> wrote:
> From: Andrei Emeltchenko <[email protected]>
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
From: Andrei Emeltchenko <[email protected]>
---
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
Hi Andrei,
* Andrei Emeltchenko <[email protected]> [2012-09-06 15:05:46 +0300]:
> From: Andrei Emeltchenko <[email protected]>
>
> Add Read Data Block Size HCI cmd to AMP initialization, then it
> makes possible to send data.
>
> Signed-off-by: Andrei Emeltchenko <[email protected]>
> ---
> net/bluetooth/hci_core.c | 3 +++
> 1 file changed, 3 insertions(+)
Patch has been applied to bluetooth-next. Thanks.
Gustavo
Hi Andrei,
* Andrei Emeltchenko <[email protected]> [2012-09-06 15:05:45 +0300]:
> From: Andrei Emeltchenko <[email protected]>
>
>
> Signed-off-by: Andrei Emeltchenko <[email protected]>
> ---
> 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
* Mat Martineau <[email protected]> [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 <[email protected]>
> >>>
> >>>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
Hi Andrei,
* Andrei Emeltchenko <[email protected]> [2012-09-06 15:05:44 +0300]:
> From: Andrei Emeltchenko <[email protected]>
>
>
> Signed-off-by: Andrei Emeltchenko <[email protected]>
> ---
> 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
Hi Andrei,
* Andrei Emeltchenko <[email protected]> [2012-09-06 15:05:43 +0300]:
> From: Andrei Emeltchenko <[email protected]>
>
> Return code is not needed in hci_chan_del
>
> Signed-off-by: Andrei Emeltchenko <[email protected]>
> ---
> include/net/bluetooth/hci_core.h | 2 +-
> net/bluetooth/hci_conn.c | 4 +---
> 2 files changed, 2 insertions(+), 4 deletions(-)
patch has been applied to bluetooth-next.git. Thanks.
Gustavo
Hi Andrei,
* Andrei Emeltchenko <[email protected]> [2012-09-06 15:05:42 +0300]:
> From: Andrei Emeltchenko <[email protected]>
>
> 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 <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
patch has been applied to bluetooth.git. Thanks.
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 <[email protected]>
>>>
>>> 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
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 <[email protected]>
> >
> > 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
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 <[email protected]>
> >
> >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 <[email protected]>
>
> 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
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 <[email protected]>
>
> 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
Andrei -
On Thu, 6 Sep 2012, Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
Hi Andrei -
On Thu, 6 Sep 2012, Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> 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 <[email protected]>
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
From: Andrei Emeltchenko <[email protected]>
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 <[email protected]>
---
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
From: Andrei Emeltchenko <[email protected]>
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
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
From: Andrei Emeltchenko <[email protected]>
Add Read Data Block Size HCI cmd to AMP initialization, then it
makes possible to send data.
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
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
From: Andrei Emeltchenko <[email protected]>
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
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
From: Andrei Emeltchenko <[email protected]>
Return code is not needed in hci_chan_del
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
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