Re: [PATCHv1 16/17] Bluetooth: A2MP: Handling fixed channels

[Mat Martineau <mathewm@codeaurora.org>]

On Tue, 22 May 2012, Andrei Emeltchenko wrote:

> Hi Mat,
>
> Thanks for review,
>
> On Mon, May 21, 2012 at 03:28:57PM -0700, Mat Martineau wrote:
>>> @@ -480,7 +487,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
>>> {
>>> 	struct sock *sk = chan->sk;
>>> 	struct l2cap_conn *conn = chan->conn;
>>> -	struct sock *parent = bt_sk(sk)->parent;
>>> +	struct sock *parent;
>>>
>>> 	__clear_chan_timer(chan);
>>>
>>> @@ -496,6 +503,9 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
>>> 		hci_conn_put(conn->hcon);
>>> 	}
>>>
>>> +	if (chan->chan_type == L2CAP_CHAN_CONN_FIX_A2MP)
>>> +		goto clean;
>>> +
>>
>> Since this has more to do with socketless channels than A2MP, it
>> might be better to do something more generic here like add a
>> callback to l2cap_ops so this socket code can be removed from
>> l2cap_core rather than worked around.
>
> This can be done. What would be good function name? l2cap_chan_del_cb,
> l2cap_chan_unlink ?
>
> The downside is that the code will be used only once ...

I think the long term goal is to get all socket code out of 
l2cap_core.c and rework RFCOMM to directly use l2cap_chan instead of 
sockets, so the callback would be used elsewhere.

>>
>>> 	lock_sock(sk);
>>>
>>> 	__l2cap_state_change(chan, BT_CLOSED);
>>> @@ -504,6 +514,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
>>> 	if (err)
>>> 		__l2cap_chan_set_err(chan, err);
>>>
>>> +	parent = bt_sk(sk)->parent;
>>> 	if (parent) {
>>> 		bt_accept_unlink(sk);
>>> 		parent->sk_data_ready(parent, 0);
>>> @@ -515,6 +526,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
>>> 	if (test_bit(CONF_NOT_COMPLETE, &chan->conf_state))
>>> 		return;
>>>
>>> +clean:
>>> 	skb_queue_purge(&chan->tx_q);
>>>
>>> 	if (chan->mode == L2CAP_MODE_ERTM) {
>>> @@ -992,6 +1004,9 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
>>> 	if (!conn)
>>> 		return;
>>>
>>> +	if (chan->chan_type == L2CAP_CHAN_CONN_FIX_A2MP)
>>> +		return;
>>> +
>>
>> There are places in the ERTM state machine that call
>> l2cap_send_disconn_req() to tear down an unrecoverable ERTM
>> connection.  If the connection remained up, I'm not sure how ERTM
>> will behave.
>
> I can rework this part so that in a case of fixed channel we do not send
> Disc Req and do not lock sk. So this would adds clearing timers and then
> return.

Ok.

>
>> I don't know that the spec is very clear about what to do when ERTM
>> must disconnect the channel, but the channel is fixed.
>
> I think we shall do nothing.
>
>> I think it would be best to tell the owner of the channel (A2MP in
>> this case) that the channel has been disconnected using the
>> state_change l2cap op, even if no disconnect request is sent to the
>> remote device.  This way a new A2MP fixed channel can be started up
>> so there is some chance that the two sides of the A2MP ERTM
>> connection can sync up again.
>
> I can add state_change and the code would look like:
>
> static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct
> l2cap_chan *chan, int err)
> {
> 	struct sock *sk = chan->sk;
> 	struct l2cap_disconn_req req;
>
> 	if (!conn)
> 		return;
>
> 	if (chan->mode == L2CAP_MODE_ERTM) {
> 		__clear_retrans_timer(chan);
> 		__clear_monitor_timer(chan);
> 		__clear_ack_timer(chan);
> 	}
>
> 	if (chan->chan_type == L2CAP_CHAN_CONN_FIX_A2MP) {
> 		__l2cap_state_change(chan, BT_DISCONN);
> 		return;
> 	}
>
> 	req.dcid = cpu_to_le16(chan->dcid);
> 	req.scid = cpu_to_le16(chan->scid);
> 	l2cap_send_cmd(conn, l2cap_get_ident(conn),
> 			L2CAP_DISCONN_REQ, sizeof(req), &req);
>
> 	lock_sock(sk);
> 	__l2cap_state_change(chan, BT_DISCONN);
> 	__l2cap_chan_set_err(chan, err);
> 	release_sock(sk);
> }

Looks fine to me.  The A2MP fixed channel is a special case in this 
regard.

>
> Then function might be renamed to l2cap_disconnect_chan()
>
>>> 	if (chan->mode == L2CAP_MODE_ERTM) {
>>> 		__clear_retrans_timer(chan);
>>> 		__clear_monitor_timer(chan);
>>> @@ -1201,6 +1216,11 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
>>>
>>> 		l2cap_chan_lock(chan);
>>>
>>> +		if (chan->chan_type == L2CAP_CHAN_CONN_FIX_A2MP) {
>>> +			l2cap_chan_unlock(chan);
>>> +			continue;
>>> +		}
>>> +
>>
>> This bypasses the socket code in that loop, but also means the state
>> change is not sent to the AMP manager.
>
> Yes, but state is already BT_CONNECTED for fixed channel so it does not
> need to change.

Since the ERTM fixed channel can be created synchronously now (unlike 
the older Code Aurora Forum code), I think you're right that the state 
change notification isn't needed here any more.

>> Is there a way to fix up the
>> state change code in l2cap_ops so the socket code can be removed
>> from l2cap_conn_ready and other socketless L2CAP channels will also
>> work without special case code?
>
> If we would invoke sk->sk_state_change(sk) each time we call state_change
> it would be easily done. Otherwise we might need special ops for
> sk_state_change.

Right.  I'm not sure why they need to be separate - there may be 
some good reason, or maybe it's just hanging around from older code.

>>> 		if (conn->hcon->type == LE_LINK) {
>>> 			if (smp_conn_security(conn, chan->sec_level))
>>> 				l2cap_chan_ready(chan);
>>> @@ -1581,7 +1601,8 @@ static void l2cap_monitor_timeout(struct work_struct *work)
>>> 	l2cap_chan_lock(chan);
>>>
>>> 	if (chan->retry_count >= chan->remote_max_tx) {
>>> -		l2cap_send_disconn_req(chan->conn, chan, ECONNABORTED);
>>> +		if (chan->chan_type != L2CAP_CHAN_CONN_FIX_A2MP)
>>> +			l2cap_send_disconn_req(chan->conn, chan, ECONNABORTED);
>>
>> The spec defines the MaxTransmit value as 0xff for the A2MP channel,
>> not 0x00 (for infinite retries).  There is therefore an expectation
>> that the ERTM connection will reset after too many retries - and I
>> don't think it is ok to skip the disconnect here.
>
> I will delete this chunk at all since we already check for fixed channel
> in l2cap_send_disconn_req



--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum