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

[Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com>]

Hi Mat,

On Fri, May 25, 2012 at 10:18:20AM -0700, Mat Martineau wrote:
> >On Thu, May 24, 2012 at 08:59:57AM -0700, Mat Martineau wrote:
> >>On Thu, 24 May 2012, Andrei Emeltchenko wrote:
> >>>On Wed, May 23, 2012 at 08:44:22AM -0700, Mat Martineau wrote:
> >>>>>>>+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
> 
> >>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.

> >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.
> 
> >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