Re: [PATCHv1 01/17] Bluetooth: A2MP: Create A2MP channel

Hi Mat,

On Fri, May 25, 2012 at 10:18:20AM -0700, Mat Martineau wrote:

quoted

On Thu, May 24, 2012 at 08:59:57AM -0700, Mat Martineau wrote:

quoted

On Thu, 24 May 2012, Andrei Emeltchenko wrote:

quoted

On Wed, May 23, 2012 at 08:44:22AM -0700, Mat Martineau wrote:

quoted

quoted

quoted

quoted

+static struct l2cap_chan *a2mp_chan_open(struct l2cap_conn *conn)
+{
+	struct l2cap_chan *chan;
+
+	chan = l2cap_chan_create();
+	if (!chan)
+		return NULL;
+
+	BT_DBG("chan %p", chan);
+
+	hci_conn_hold(conn->hcon);

Holding the hcon will keep the ACL open after all of the other L2CAP
channels have closed (unless I missed some code later in the patch
series).  The A2MP fixed channel should not keep the ACL open.  If
the connection is not held here, then there shouldn't be a put in
l2cap_chan_del for the A2MP channel either.

l2cap_chan_del makes hci_conn_put, also for a2mp channel. The behavior is
the same for fixed and normal channels.

And when does l2cap_chan_del get called for a fixed channel?  The
fixed channel must not do an hci_conn_hold so the ACL is allowed to
close when all dynamic L2CAP channels have closed.

The current approach is to have amp_mgr for hci_conn. It will be freed
when in hci_conn_del together with other l2cap channels in
hci_chan_list_flush and then we make amp_mgr_put() and destroy mgr.

The idea is to make a2mp chan similar to other chans and other chans do
hci_conn_hold and hci_conn_put. Otherwise I would need to add extra checks
before hci_conn_put which is ugly.

I've tested this with PTS and no memory leaks found so far.

The risk is not memory leaks - if the other device closes the ACL,
then the A2MP channel will be cleaned up.  But if you had two BlueZ
devices with A2MP connected to each other, the ACL would never close
as long as the devices remained powered and in range!  Nothing would
trigger the flush as long as the hci_conn is held.

Does it indicate that we have problem with rather freeing amp_mgr?

It looks like the amp_mgr will be freed if the channel is closed.
There's nothing that will close the channel except for:

* Remote device disconnects the ACL
* Remote device goes out of range
* Local BR/EDR controller is powered down or reset

quoted

quoted

I think the checks for hci_conn_put are necessary, even if they are
ugly.

I think that if I have hci_conn_hold when creating channel and
hci_conn_put when closing channel that does not differ from the case when
I do not have hci_conn_put/hold.

If there was something that closed the A2MP channel, there would be
no difference.  But nothing closes the A2MP channel.

I actually was thinking about timers which could do clean up.

quoted

So you are probably referring to the case when hci_conn needs to be closed
but cannot because it has hci_conn_hold in a2mp channel?

Yes.

quoted

This also means that we still have L2CAP channel (a2mp) hanging.
This seems right to me if I did not miss something.

Since A2MP is a fixed channel, it is not opened and closed like
typical L2CAP channels.  Consider this scenario:

1. Remote device decides to connect
2. ACL is created
3. Remote device sends A2MP discover
4. A2MP channel is set up, AMP manager is created, hci_conn_hold for
A2MP channel
5. Remote device connects L2CAP channel over BR/EDR.  hci_conn_hold
for the data channel
6. A2MP communication to set up physical link
7. L2CAP channel move to AMP controller
8. Data sent over AMP.
9. L2CAP channel disconnected, hci_conn_put for the data channel
10. Device stays in range.  hci_conn reference count is not 0, so
ACL is not closed.

My concern here if A2MP does not reference hci_conn can hci_conn be
removed while we still have A2MP chan in some situation?

I will modify code that A2MP does not reference hci_conn, or create
special patch with this change.

Best regards 
Andrei Emeltchenko