From: Gustavo Padovan <gustavo@padovan.org>
To: Mat Martineau <mathewm@codeaurora.org>
Cc: Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com>,
Marcel Holtmann <marcel@holtmann.org>,
linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH 1/6] Bluetooth: Fix deadlock when closing socket
Date: Sat, 8 Sep 2012 18:05:02 -0300 [thread overview]
Message-ID: <20120908210502.GH5788@joana> (raw)
In-Reply-To: <alpine.DEB.2.02.1209070949580.3761@mathewm-linux>
* 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
next prev parent reply other threads:[~2012-09-08 21:05 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
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 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
2012-09-08 20:33 ` Gustavo Padovan
2012-09-06 12:05 ` [PATCH 4/6] Bluetooth: trivial: Remove empty line 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
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-08 21:14 ` Gustavo Padovan
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 17:00 ` Mat Martineau
2012-09-08 21:05 ` Gustavo Padovan [this message]
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
2012-09-07 14:07 ` [PATCH 1/6] Bluetooth: Fix deadlock when closing socket Andrei Emeltchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120908210502.GH5788@joana \
--to=gustavo@padovan.org \
--cc=Andrei.Emeltchenko.news@gmail.com \
--cc=linux-bluetooth@vger.kernel.org \
--cc=marcel@holtmann.org \
--cc=mathewm@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).