Hi Andrei,
On Tue, Feb 21, 2012 at 8:55 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@xxxxxxxxx> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@xxxxxxxxx>
>
> Convert sk lock to chan lock for L2CAP signalling commands.
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@xxxxxxxxx>
> ---
> net/bluetooth/l2cap_core.c | 61 +++++++++++++++++++++++++------------------
> net/bluetooth/l2cap_sock.c | 7 +++-
> 2 files changed, 40 insertions(+), 28 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index c008608..ab6bede 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -377,6 +377,8 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
> hci_conn_put(conn->hcon);
> }
>
> + lock_sock(sk);
> +
> __l2cap_state_change(chan, BT_CLOSED);
> sock_set_flag(sk, SOCK_ZAPPED);
>
> @@ -389,6 +391,8 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
> } else
> sk->sk_state_change(sk);
>
> + release_sock(sk);
> +
> if (!(test_bit(CONF_OUTPUT_DONE, &chan->conf_state) &&
> test_bit(CONF_INPUT_DONE, &chan->conf_state)))
> return;
You're making l2cap_chan_del() deal with sock lock so please remove
the comment above the function signature. It doesn't make sense to
leave old comments around.
> @@ -421,10 +425,10 @@ static void l2cap_chan_cleanup_listen(struct sock *parent)
> while ((sk = bt_accept_dequeue(parent, NULL))) {
> struct l2cap_chan *chan = l2cap_pi(sk)->chan;
>
> + l2cap_chan_lock(chan);
> __clear_chan_timer(chan);
> - lock_sock(sk);
> l2cap_chan_close(chan, ECONNRESET);
> - release_sock(sk);
> + l2cap_chan_unlock(chan);
>
> chan->ops->close(chan->data);
> }
Ok.
> @@ -440,10 +444,12 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
>
> switch (chan->state) {
> case BT_LISTEN:
> + lock_sock(sk);
> l2cap_chan_cleanup_listen(sk);
>
> __l2cap_state_change(chan, BT_CLOSED);
> sock_set_flag(sk, SOCK_ZAPPED);
> + release_sock(sk);
> break;
>
> case BT_CONNECTED:
Do we need to lock sock around l2cap_chan_cleanup_listen() as well?
l2cap_chan_close() will call l2cap_chan_del() which now does
lock_sock/release_sock, right?
> @@ -486,7 +492,9 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
> break;
>
> default:
> + lock_sock(sk);
> sock_set_flag(sk, SOCK_ZAPPED);
> + release_sock(sk);
> break;
> }
> }
Hmm, this indeed looks correct. However, it'd be good to later revisit
this as we want core without any sock handling, right?
> @@ -714,6 +722,7 @@ static inline int l2cap_mode_supported(__u8 mode, __u32 feat_mask)
>
> 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)
> @@ -730,8 +739,10 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
> 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);
> }
>
> /* ---- L2CAP connections ---- */
It seems we didn't have any locking around these before. Why? Do we
really need it now?
<snip>
Regards,
--
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs
--
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
