Hi Marcel,
On Wed, Oct 17, 2012 at 10:16:27AM -0700, 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@xxxxxxxxxxxxxx>
> > ---
> > 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 9663292..ed2c23f 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -1036,6 +1036,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 */
> > @@ -4302,11 +4338,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))
> > @@ -4317,8 +4354,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);
> > +
>
> still not a big fan of the if (chan) check before the unlock. This way
> of dealing with locks makes my brain hurt ;)
I think in this case the solution would be to invoke
l2cap_send_move_chan_cfm_rsp 2 times: 1st time atfer checking (!chan) and
second time here. Since this message does not need any error code this
looks OK to me.
Best regards
Andrei Emeltchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
