From: Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com>
To: Marcel Holtmann <marcel@holtmann.org>
Cc: Mat Martineau <mathewm@codeaurora.org>,
linux-bluetooth@vger.kernel.org, gustavo@padovan.org,
sunnyk@codeaurora.org
Subject: Re: [PATCHv2 07/19] Bluetooth: Add move channel confirm handling
Date: Thu, 18 Oct 2012 10:48:54 +0300 [thread overview]
Message-ID: <20121018074848.GE13545@aemeltch-MOBL1> (raw)
In-Reply-To: <1350494187.26318.119.camel@aeonflux>
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@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 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
next prev parent reply other threads:[~2012-10-18 7:48 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-16 23:36 [PATCHv2 00/19] L2CAP signaling for AMP channel create/move Mat Martineau
2012-10-16 23:36 ` [PATCHv2 01/19] Bluetooth: Add new l2cap_chan struct members for high speed channels Mat Martineau
2012-10-16 23:36 ` [PATCHv2 02/19] Bluetooth: Add L2CAP create channel request handling Mat Martineau
2012-10-16 23:36 ` [PATCHv2 03/19] Bluetooth: Remove unnecessary intermediate function Mat Martineau
2012-10-16 23:36 ` [PATCHv2 04/19] Bluetooth: Lookup channel structure based on DCID Mat Martineau
2012-10-17 17:12 ` Marcel Holtmann
2012-10-18 7:30 ` Andrei Emeltchenko
2012-10-18 15:51 ` Mat Martineau
2012-10-16 23:36 ` [PATCHv2 05/19] Bluetooth: Channel move request handling Mat Martineau
2012-10-18 7:38 ` Andrei Emeltchenko
2012-10-16 23:36 ` [PATCHv2 06/19] Bluetooth: Add new ERTM receive states for channel move Mat Martineau
2012-10-17 17:13 ` Marcel Holtmann
2012-10-16 23:36 ` [PATCHv2 07/19] Bluetooth: Add move channel confirm handling Mat Martineau
2012-10-17 17:16 ` Marcel Holtmann
2012-10-18 7:48 ` Andrei Emeltchenko [this message]
2012-10-16 23:36 ` [PATCHv2 08/19] Bluetooth: Add state to hci_chan Mat Martineau
2012-10-16 23:36 ` [PATCHv2 09/19] Bluetooth: Move channel response Mat Martineau
2012-10-17 17:20 ` Marcel Holtmann
2012-10-16 23:36 ` [PATCHv2 10/19] Bluetooth: Add logical link confirm Mat Martineau
2012-10-17 17:22 ` Marcel Holtmann
2012-10-18 8:36 ` Andrei Emeltchenko
2012-10-16 23:36 ` [PATCHv2 11/19] Bluetooth: Add move confirm response handling Mat Martineau
2012-10-16 23:36 ` [PATCHv2 12/19] Bluetooth: Handle physical link completion Mat Martineau
2012-10-16 23:36 ` [PATCHv2 13/19] Bluetooth: Flag ACL frames as complete for AMP controllers Mat Martineau
2012-10-16 23:36 ` [PATCHv2 14/19] Bluetooth: Do not send data during channel move Mat Martineau
2012-10-16 23:36 ` [PATCHv2 15/19] Bluetooth: Configure appropriate timeouts for AMP controllers Mat Martineau
2012-10-16 23:36 ` [PATCHv2 16/19] Bluetooth: Ignore BR/EDR packet size constraints when fragmenting for AMP Mat Martineau
2012-10-16 23:36 ` [PATCHv2 17/19] Bluetooth: Send create channel request instead of connect " Mat Martineau
2012-10-17 17:24 ` Marcel Holtmann
2012-10-18 9:40 ` Andrei Emeltchenko
2012-10-18 16:04 ` Mat Martineau
2012-10-16 23:36 ` [PATCHv2 18/19] Bluetooth: Do not retransmit data during a channel move Mat Martineau
2012-10-16 23:36 ` [PATCHv2 19/19] Bluetooth: Start channel move when socket option is changed Mat Martineau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20121018074848.GE13545@aemeltch-MOBL1 \
--to=andrei.emeltchenko.news@gmail.com \
--cc=gustavo@padovan.org \
--cc=linux-bluetooth@vger.kernel.org \
--cc=marcel@holtmann.org \
--cc=mathewm@codeaurora.org \
--cc=sunnyk@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).