Re: [RFCv5 03/26] Bluetooth: A2MP: Create A2MP channel

[Gustavo Padovan <gustavo@padovan.org>]

Hi Andrei,

* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-03-26 12:27:39 +0300]:

> Hi Gustavo,
> 
> On Sun, Mar 25, 2012 at 02:12:25PM -0300, Gustavo Padovan wrote:
> > > +static struct l2cap_chan *open_a2mp_chan(struct l2cap_conn *conn)
> > 
> > a2mp_chan_open() is a better name here. Who is calling this btw?
> 
> Agree, will change it; it is called by amp_mgr_create in the following
> patch.
> 
> > > +{
> > > +	struct l2cap_chan *chan;
> > > +
> > > +	chan = l2cap_chan_create(NULL);
> > 
> > I just pushed a patch that remove the sk parameter from l2cap_chan_create(),
> > so you don't need to pass NULL here anymore.
> 
> Good! I did not know about this. BTW: Shall all patches go through mailing list
> first?

I committed just after see your patch, it was a simple patch.

> 
> > You failing to check l2cap_chan_create() for NULL return btw.
> 
> Will fix.
> 
> > > +
> > > +	hci_conn_hold(conn->hcon);
> > > +
> > > +	BT_DBG("chan %p", chan);
> > > +
> > > +	chan->sec_level = BT_SECURITY_LOW;
> > > +	chan->omtu = L2CAP_A2MP_DEFAULT_MTU;
> > > +	chan->imtu = L2CAP_A2MP_DEFAULT_MTU;
> > > +	chan->flush_to = L2CAP_DEFAULT_FLUSH_TO;
> > > +	chan->fcs = L2CAP_FCS_CRC16;
> > > +
> > > +	chan->ops = &a2mp_chan_ops;
> > > +
> > > +	set_bit(FLAG_FORCE_ACTIVE, &chan->flags);
> > > +
> > > +	chan->max_tx = L2CAP_DEFAULT_MAX_TX;
> > > +	chan->remote_max_tx = chan->max_tx;
> > > +
> > > +	chan->tx_win = L2CAP_DEFAULT_TX_WINDOW;
> > > +	chan->tx_win_max = L2CAP_DEFAULT_TX_WINDOW;
> > > +	chan->remote_tx_win = chan->tx_win;
> > > +
> > > +	chan->retrans_timeout = L2CAP_DEFAULT_RETRANS_TO;
> > > +	chan->monitor_timeout = L2CAP_DEFAULT_MONITOR_TO;
> > > +
> > > +	skb_queue_head_init(&chan->tx_q);
> > 
> > I think we should move all ERTM related settings here inside
> > l2cap_ertm_init(), it will make this code much more cleaner.
> 
> which settings? retrans_timeout and monitor_timeout are set in
> configuration phase before l2cap_ertm_init and putting them
> to l2cap_ertm_init would override them.
> 
> I can make a function assigning basic L2CAP settings for A2MP and for
> general L2CAP channels (those in the beginning).

Ok, let's go with this approach then.

	Gustavo