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 ...
>
> > 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.
> 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);
}
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.
> 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.
> > 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
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
