Re: [PATCH 07/19] Bluetooth: Add move channel confirm handling

[Mat Martineau <mathewm@codeaurora.org>]

Hi Marcel -

On Mon, 15 Oct 2012, Marcel Holtmann wrote:

> Hi Mat,
>
>> After sending a move channel response, a move responder waits for a
>> move channel confirm command.  If the received command has a
>> "confirmed" result the move is proceeding, and "unconfirmed" means the
>> move has failed and the channel will not change controllers.
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  net/bluetooth/l2cap_core.c | 70 ++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 67 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index aab7f79..ef744a9 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -1030,6 +1030,42 @@ static void l2cap_move_setup(struct l2cap_chan *chan)
>>  	set_bit(CONN_REMOTE_BUSY, &chan->conn_state);
>>  }
>>
>> +static void l2cap_move_success(struct l2cap_chan *chan)
>> +{
>> +	BT_DBG("chan %p", chan);
>> +
>> +	if (chan->mode != L2CAP_MODE_ERTM)
>> +		return;
>> +
>> +	switch (chan->move_role) {
>> +	case L2CAP_MOVE_ROLE_INITIATOR:
>> +		l2cap_tx(chan, NULL, NULL, L2CAP_EV_EXPLICIT_POLL);
>> +		chan->rx_state = L2CAP_RX_STATE_WAIT_F;
>> +		break;
>> +	case L2CAP_MOVE_ROLE_RESPONDER:
>> +		chan->rx_state = L2CAP_RX_STATE_WAIT_P;
>> +		break;
>> +	}
>> +}
>> +
>> +static void l2cap_move_revert(struct l2cap_chan *chan)
>> +{
>> +	BT_DBG("chan %p", chan);
>> +
>> +	if (chan->mode != L2CAP_MODE_ERTM)
>> +		return;
>> +
>> +	switch (chan->move_role) {
>> +	case L2CAP_MOVE_ROLE_INITIATOR:
>> +		l2cap_tx(chan, NULL, NULL, L2CAP_EV_EXPLICIT_POLL);
>> +		chan->rx_state = L2CAP_RX_STATE_WAIT_F;
>> +		break;
>> +	case L2CAP_MOVE_ROLE_RESPONDER:
>> +		chan->rx_state = L2CAP_RX_STATE_WAIT_P;
>> +		break;
>> +	}
>> +}
>> +
>>  static void l2cap_chan_ready(struct l2cap_chan *chan)
>>  {
>>  	/* This clears all conf flags, including CONF_NOT_COMPLETE */
>> @@ -4297,11 +4333,12 @@ static inline int l2cap_move_channel_rsp(struct l2cap_conn *conn,
>>  	return 0;
>>  }
>>
>> -static inline int l2cap_move_channel_confirm(struct l2cap_conn *conn,
>> -					     struct l2cap_cmd_hdr *cmd,
>> -					     u16 cmd_len, void *data)
>> +static int l2cap_move_channel_confirm(struct l2cap_conn *conn,
>> +				      struct l2cap_cmd_hdr *cmd,
>> +				      u16 cmd_len, void *data)
>>  {
>>  	struct l2cap_move_chan_cfm *cfm = data;
>> +	struct l2cap_chan *chan;
>>  	u16 icid, result;
>>
>>  	if (cmd_len != sizeof(*cfm))
>> @@ -4312,8 +4349,35 @@ static inline int l2cap_move_channel_confirm(struct l2cap_conn *conn,
>>
>>  	BT_DBG("icid 0x%4.4x, result 0x%4.4x", icid, result);
>>
>> +	chan = l2cap_get_chan_by_dcid(conn, icid);
>> +	if (!chan)
>> +		goto send_move_confirm_response;
>> +
>> +	if (chan->move_state == L2CAP_MOVE_WAIT_CONFIRM) {
>> +		chan->move_state = L2CAP_MOVE_STABLE;
>> +		if (result == L2CAP_MC_CONFIRMED) {
>> +			chan->local_amp_id = chan->move_id;
>> +			if (!chan->local_amp_id) {
>> +				/* Have moved off of AMP, free the channel */
>> +				chan->hs_hchan = NULL;
>> +				chan->hs_hcon = NULL;
>> +
>> +				/* Placeholder - free the logical link */
>> +			}
>> +			l2cap_move_success(chan);
>> +		} else {
>> +			chan->move_id = chan->local_amp_id;
>> +			l2cap_move_revert(chan);
>> +		}
>> +		chan->move_role = L2CAP_MOVE_ROLE_NONE;
>> +	}
>> +
>> +send_move_confirm_response:
>>  	l2cap_send_move_chan_cfm_rsp(conn, cmd->ident, icid);
>>
>> +	if (chan)
>> +		l2cap_chan_unlock(chan);
>> +
>>  	return 0;
>>  }
>>
>
> the more I read into this one, can we not have a clean exit when (!chan)
> for this one. Or am I missing something.

Are you thinking this should send a COMMAND_REJ if it can't find the 
matching channel?

I think sending the confirm response is the appropriate thing to do. 
The spec says "When a device receives a Move Channel Confirmation 
packet it shall send a Move Channel Confirmation response packet".
Sending this response doesn't indicate that the move was successful.

--
Mat Martineau

Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation