Re: [RFCv0 10/21] Bluetooth: Add logical link confirm

[Mat Martineau <mathewm@codeaurora.org>]

Hi Andrei -

On Thu, 26 Jul 2012, Andrei Emeltchenko wrote:

> Hi Mat,
>
> On Wed, Jul 25, 2012 at 04:51:02PM -0700, Mat Martineau wrote:
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  include/net/bluetooth/l2cap.h |   1 +
>>  net/bluetooth/l2cap_core.c    | 120 +++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 119 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> index fcc971a..7c50606 100644
>> --- a/include/net/bluetooth/l2cap.h
>> +++ b/include/net/bluetooth/l2cap.h
>> @@ -461,6 +461,7 @@ struct l2cap_chan {
>>
>>  	__u8		conf_req[64];
>>  	__u8		conf_len;
>> +	__u8		conf_ident;
>>  	__u8		num_conf_req;
>>  	__u8		num_conf_rsp;
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index 8d72f2e..79f9d8e 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -3717,6 +3717,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>>  		goto unlock;
>>  	}
>>
>> +	chan->conf_ident = cmd->ident;
>>  	l2cap_send_cmd(conn, cmd->ident, L2CAP_CONF_RSP, len, rsp);
>>  	chan->num_conf_rsp++;
>>
>> @@ -4161,11 +4162,126 @@ static void l2cap_send_move_chan_cfm_rsp(struct l2cap_conn *conn, u8 ident,
>>  	l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_CFM_RSP, sizeof(rsp), &rsp);
>>  }
>>
>> +/* Call with chan locked */
>>  static void l2cap_logical_cfm(struct l2cap_chan *chan, struct hci_chan *hchan,
>>  			      u8 status)
>>  {
>> -	/* Placeholder */
>> -	return;
>> +	BT_DBG("chan %p, hchan %p, status %d", chan, hchan, status);
>> +
>> +	if (chan->state != BT_CONNECTED && !chan->chan_id)
>> +		return;
>> +
>> +	if (chan && !status && chan->state != BT_CONNECTED) {
>
> If we consider chan might be NULL we would crash in the previous reference
> of chan->state.
>

I'll fix that.

>> +		struct l2cap_conf_rsp rsp;
>> +		u8 code;
>> +
>> +		/* Create channel complete */
>> +		chan->hs_hcon = hchan->conn;
>> +		chan->hs_hcon->l2cap_data = chan->conn;
>> +
>> +		code = l2cap_build_conf_rsp(chan, &rsp,
>> +					    L2CAP_CONF_SUCCESS, 0);
>> +		l2cap_send_cmd(chan->conn, chan->conf_ident,
>> +			       L2CAP_CONF_RSP, code, &rsp);
>> +		set_bit(CONF_OUTPUT_DONE, &chan->conf_state);
>> +
>> +		if (test_bit(CONF_INPUT_DONE, &chan->conf_state)) {
>> +			int err = 0;
>> +
>> +			set_default_fcs(chan);
>> +
>> +			if (chan->mode == L2CAP_MODE_ERTM ||
>> +			    chan->mode == L2CAP_MODE_STREAMING)
>> +				err = l2cap_ertm_init(chan);
>> +
>> +			if (err < 0)
>> +				l2cap_send_disconn_req(chan->conn, chan, -err);
>> +			else
>> +				l2cap_chan_ready(chan);
>> +		}
>> +	} else if (chan && !status) {
>
> Otherwise we always reference chan so we might check for chan in the
> beginning.
>
>> +		/* Channel move */
>> +		chan->hs_hcon = hchan->conn;
>> +		chan->hs_hcon->l2cap_data = chan->conn;
>> +
>> +		BT_DBG("move_state %d", chan->move_state);
>
> in the future patches I would prefer something similar to state_to_string.
>
> ...

Future patches that print move_state?  (This is now the only place 
that prints it)

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