From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <1329821707-11817-9-git-send-email-Andrei.Emeltchenko.news@gmail.com> References: <1329821707-11817-1-git-send-email-Andrei.Emeltchenko.news@gmail.com> <1329821707-11817-9-git-send-email-Andrei.Emeltchenko.news@gmail.com> Date: Tue, 21 Feb 2012 15:18:45 -0200 Message-ID: Subject: Re: [PATCHv1 08/14] Bluetooth: Use chan lock in L2CAP sig commands From: Ulisses Furquim To: Andrei Emeltchenko Cc: linux-bluetooth@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Andrei, On Tue, Feb 21, 2012 at 8:55 AM, Andrei Emeltchenko wrote: > From: Andrei Emeltchenko > > Convert sk lock to chan lock for L2CAP signalling commands. > > Signed-off-by: Andrei Emeltchenko > --- >  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? Regards, -- Ulisses Furquim ProFUSION embedded systems http://profusion.mobi Mobile: +55 19 9250 0942 Skype: ulissesffs