[RFCv0 0/5] Bluetooth: Change socket lock to l2cap_chan lock

[RFCv0 0/5] Bluetooth: Change socket lock to l2cap_chan lock

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

This is DRAFT RFC introducing l2cap channel lock. Please make suggestion how to
make it better.

Beside changing socket locks to l2cap_chan lock channel list lock is reverted.
It is now used to protect RCU updaters or RCU lists which might sleep and not
protected by rcu_read_lock.

Andrei Emeltchenko (5):
  Bluetooth: Use locks in RCU updater code
  Bluetooth: Add l2cap_chan_lock
  Bluetooth: Helper functions for locking change
  Bluetooth: Remove unneeded sk variable
  Bluetooth: Change sk lock to l2cap_chan lock

 include/net/bluetooth/l2cap.h |   11 ++
 net/bluetooth/l2cap_core.c    |  267 ++++++++++++++++++++++++++---------------
 net/bluetooth/l2cap_sock.c    |   13 ++-
 3 files changed, 193 insertions(+), 98 deletions(-)

-- 
1.7.4.1

[RFCv0 1/5] Bluetooth: Use locks in RCU updater code

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Code which makes changes to RCU list shall be locked.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 6991821..f54768e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -743,13 +743,13 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
 /* ---- L2CAP connections ---- */
 static void l2cap_conn_start(struct l2cap_conn *conn)
 {
-	struct l2cap_chan *chan;
+	struct l2cap_chan *chan, *tmp;
 
 	BT_DBG("conn %p", conn);
 
-	rcu_read_lock();
+	mutex_lock(&conn->chan_lock);
 
-	list_for_each_entry_rcu(chan, &conn->chan_l, list) {
+	list_for_each_entry_safe(chan, tmp, &conn->chan_l, list) {
 		struct sock *sk = chan->sk;
 
 		bh_lock_sock(sk);
@@ -829,7 +829,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 		bh_unlock_sock(sk);
 	}
 
-	rcu_read_unlock();
+	mutex_unlock(&conn->chan_lock);
 }
 
 /* Find socket with cid and source bdaddr.
@@ -1009,6 +1009,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 
 	kfree_skb(conn->rx_skb);
 
+	mutex_lock(&conn->chan_lock);
+
 	/* Kill channels */
 	list_for_each_entry_safe(chan, l, &conn->chan_l, list) {
 		sk = chan->sk;
@@ -1018,6 +1020,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 		chan->ops->close(chan->data);
 	}
 
+	mutex_unlock(&conn->chan_lock);
+
 	hci_chan_del(conn->hchan);
 
 	if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
@@ -1075,6 +1079,7 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
 	conn->feat_mask = 0;
 
 	spin_lock_init(&conn->lock);
+	mutex_init(&conn->chan_lock);
 
 	INIT_LIST_HEAD(&conn->chan_l);
 
-- 
1.7.4.1

Re: [RFCv0 1/5] Bluetooth: Use locks in RCU updater code

[Ulisses Furquim]

Hi Andrei,

On Mon, Jan 30, 2012 at 1:09 PM, Emeltchenko Andrei
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Code which makes changes to RCU list shall be locked.
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 6991821..f54768e 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -743,13 +743,13 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
>  /* ---- L2CAP connections ---- */
>  static void l2cap_conn_start(struct l2cap_conn *conn)
>  {
> -       struct l2cap_chan *chan;
> +       struct l2cap_chan *chan, *tmp;
>
>        BT_DBG("conn %p", conn);
>
> -       rcu_read_lock();
> +       mutex_lock(&conn->chan_lock);
>
> -       list_for_each_entry_rcu(chan, &conn->chan_l, list) {
> +       list_for_each_entry_safe(chan, tmp, &conn->chan_l, list) {
>                struct sock *sk = chan->sk;
>
>                bh_lock_sock(sk);
> @@ -829,7 +829,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
>                bh_unlock_sock(sk);
>        }
>
> -       rcu_read_unlock();
> +       mutex_unlock(&conn->chan_lock);
>  }
>
>  /* Find socket with cid and source bdaddr.
> @@ -1009,6 +1009,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
>
>        kfree_skb(conn->rx_skb);
>
> +       mutex_lock(&conn->chan_lock);
> +
>        /* Kill channels */
>        list_for_each_entry_safe(chan, l, &conn->chan_l, list) {
>                sk = chan->sk;
> @@ -1018,6 +1020,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
>                chan->ops->close(chan->data);
>        }
>
> +       mutex_unlock(&conn->chan_lock);
> +
>        hci_chan_del(conn->hchan);
>
>        if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
> @@ -1075,6 +1079,7 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
>        conn->feat_mask = 0;
>
>        spin_lock_init(&conn->lock);
> +       mutex_init(&conn->chan_lock);
>
>        INIT_LIST_HEAD(&conn->chan_l);
>

I was under the impression you'd remove RCU for conn->chan_l
completely. You're adding a lock only in the updaters? If so, please
take a look at commit 3d57dc680 which shows all changes from mutex to
RCU. I don't think just adding a lock/unlock in l2cap_conn_start and
l2cap_conn_del will be enough. l2cap_chan_add seems to be called from
other contexts and it does a list_add_rcu(). Have you thought of that?

Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [RFCv0 1/5] Bluetooth: Use locks in RCU updater code

[Emeltchenko Andrei]

Hi Ulisses,

On Mon, Jan 30, 2012 at 03:17:15PM -0200, Ulisses Furquim wrote:
> Hi Andrei,
> 
> On Mon, Jan 30, 2012 at 1:09 PM, Emeltchenko Andrei
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > Code which makes changes to RCU list shall be locked.
> >
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  net/bluetooth/l2cap_core.c |   13 +++++++++----
> >  1 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index 6991821..f54768e 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -743,13 +743,13 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
> >  /* ---- L2CAP connections ---- */
> >  static void l2cap_conn_start(struct l2cap_conn *conn)
> >  {
> > -       struct l2cap_chan *chan;
> > +       struct l2cap_chan *chan, *tmp;
> >
> >        BT_DBG("conn %p", conn);
> >
> > -       rcu_read_lock();
> > +       mutex_lock(&conn->chan_lock);
> >
> > -       list_for_each_entry_rcu(chan, &conn->chan_l, list) {
> > +       list_for_each_entry_safe(chan, tmp, &conn->chan_l, list) {
> >                struct sock *sk = chan->sk;
> >
> >                bh_lock_sock(sk);
> > @@ -829,7 +829,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
> >                bh_unlock_sock(sk);
> >        }
> >
> > -       rcu_read_unlock();
> > +       mutex_unlock(&conn->chan_lock);
> >  }
> >
> >  /* Find socket with cid and source bdaddr.
> > @@ -1009,6 +1009,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
> >
> >        kfree_skb(conn->rx_skb);
> >
> > +       mutex_lock(&conn->chan_lock);
> > +
> >        /* Kill channels */
> >        list_for_each_entry_safe(chan, l, &conn->chan_l, list) {
> >                sk = chan->sk;
> > @@ -1018,6 +1020,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
> >                chan->ops->close(chan->data);
> >        }
> >
> > +       mutex_unlock(&conn->chan_lock);
> > +
> >        hci_chan_del(conn->hchan);
> >
> >        if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
> > @@ -1075,6 +1079,7 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
> >        conn->feat_mask = 0;
> >
> >        spin_lock_init(&conn->lock);
> > +       mutex_init(&conn->chan_lock);
> >
> >        INIT_LIST_HEAD(&conn->chan_l);
> >
> 
> I was under the impression you'd remove RCU for conn->chan_l
> completely. You're adding a lock only in the updaters? If so, please
> take a look at commit 3d57dc680 which shows all changes from mutex to
> RCU. I don't think just adding a lock/unlock in l2cap_conn_start and
> l2cap_conn_del will be enough. l2cap_chan_add seems to be called from
> other contexts and it does a list_add_rcu(). Have you thought of that?

I am adding lock to updaters and to the places we need to sleep and
rcu_read_lock cannot be used. This patch adds locks to updaters and
following patches cover other places. Maybe I need to split them better.

Best regards 
Andrei Emeltchenko 

Re: [RFCv0 1/5] Bluetooth: Use locks in RCU updater code

[Ulisses Furquim]

Hi Andrei,

On Tue, Jan 31, 2012 at 5:59 AM, Emeltchenko Andrei
<Andrei.Emeltchenko.news@gmail.com> wrote:
> On Mon, Jan 30, 2012 at 03:17:15PM -0200, Ulisses Furquim wrote:

<snip>

>> I was under the impression you'd remove RCU for conn->chan_l
>> completely. You're adding a lock only in the updaters? If so, please
>> take a look at commit 3d57dc680 which shows all changes from mutex to
>> RCU. I don't think just adding a lock/unlock in l2cap_conn_start and
>> l2cap_conn_del will be enough. l2cap_chan_add seems to be called from
>> other contexts and it does a list_add_rcu(). Have you thought of that?
>
> I am adding lock to updaters and to the places we need to sleep and
> rcu_read_lock cannot be used. This patch adds locks to updaters and
> following patches cover other places. Maybe I need to split them better.

It needs to be split better, yes. And if you're adding a mutex also in
some readers of the list instead of using RCU I believe it'd be better
to just use a mutex and remove RCU usage altogether. That will be
possibly just a revert of 3d57dc6806, but you need to check that.

Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [RFCv0 1/5] Bluetooth: Use locks in RCU updater code

[Emeltchenko Andrei]

Hi Ulisses,

On Tue, Jan 31, 2012 at 10:37:38AM -0200, Ulisses Furquim wrote:
> On Tue, Jan 31, 2012 at 5:59 AM, Emeltchenko Andrei
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > On Mon, Jan 30, 2012 at 03:17:15PM -0200, Ulisses Furquim wrote:
> >> I was under the impression you'd remove RCU for conn->chan_l
> >> completely. You're adding a lock only in the updaters? If so, please
> >> take a look at commit 3d57dc680 which shows all changes from mutex to
> >> RCU. I don't think just adding a lock/unlock in l2cap_conn_start and
> >> l2cap_conn_del will be enough. l2cap_chan_add seems to be called from
> >> other contexts and it does a list_add_rcu(). Have you thought of that?
> >
> > I am adding lock to updaters and to the places we need to sleep and
> > rcu_read_lock cannot be used. This patch adds locks to updaters and
> > following patches cover other places. Maybe I need to split them better.
> 
> It needs to be split better, yes. And if you're adding a mutex also in
> some readers of the list instead of using RCU I believe it'd be better
> to just use a mutex and remove RCU usage altogether. That will be
> possibly just a revert of 3d57dc6806, but you need to check that.

I actually do think that this will be better, in next patches I will
revert the commit mentioned.

Best regards 
Andrei Emeltchenko 

[RFCv0 2/5] Bluetooth: Add l2cap_chan_lock

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Channel lock will be used to lock L2CAP channels which are locked
currently by socket locks.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/l2cap.h |   11 +++++++++++
 net/bluetooth/l2cap_core.c    |    2 ++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index e7a8cc7..e81f235 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -497,6 +497,7 @@ struct l2cap_chan {
 
 	void		*data;
 	struct l2cap_ops *ops;
+	struct mutex		lock;
 };
 
 struct l2cap_ops {
@@ -609,6 +610,16 @@ static inline void l2cap_chan_put(struct l2cap_chan *c)
 		kfree(c);
 }
 
+static inline void l2cap_chan_lock(struct l2cap_chan *chan)
+{
+	mutex_lock(&chan->lock);
+}
+
+static inline void l2cap_chan_unlock(struct l2cap_chan *chan)
+{
+	mutex_unlock(&chan->lock);
+}
+
 static inline void l2cap_set_timer(struct l2cap_chan *chan,
 					struct delayed_work *work, long timeout)
 {
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index f54768e..9a23b19 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -285,6 +285,8 @@ struct l2cap_chan *l2cap_chan_create(struct sock *sk)
 	if (!chan)
 		return NULL;
 
+	mutex_init(&chan->lock);
+
 	chan->sk = sk;
 
 	write_lock(&chan_list_lock);
-- 
1.7.4.1

Re: [RFCv0 2/5] Bluetooth: Add l2cap_chan_lock

[Ulisses Furquim]

Hi Andrei,

On Mon, Jan 30, 2012 at 1:09 PM, Emeltchenko Andrei
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Channel lock will be used to lock L2CAP channels which are locked
> currently by socket locks.
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  include/net/bluetooth/l2cap.h |   11 +++++++++++
>  net/bluetooth/l2cap_core.c    |    2 ++
>  2 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index e7a8cc7..e81f235 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -497,6 +497,7 @@ struct l2cap_chan {
>
>        void            *data;
>        struct l2cap_ops *ops;
> +       struct mutex            lock;
>  };
>
>  struct l2cap_ops {
> @@ -609,6 +610,16 @@ static inline void l2cap_chan_put(struct l2cap_chan *c)
>                kfree(c);
>  }
>
> +static inline void l2cap_chan_lock(struct l2cap_chan *chan)
> +{
> +       mutex_lock(&chan->lock);
> +}
> +
> +static inline void l2cap_chan_unlock(struct l2cap_chan *chan)
> +{
> +       mutex_unlock(&chan->lock);
> +}
> +
>  static inline void l2cap_set_timer(struct l2cap_chan *chan,
>                                        struct delayed_work *work, long timeout)
>  {
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index f54768e..9a23b19 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -285,6 +285,8 @@ struct l2cap_chan *l2cap_chan_create(struct sock *sk)
>        if (!chan)
>                return NULL;
>
> +       mutex_init(&chan->lock);
> +
>        chan->sk = sk;
>
>        write_lock(&chan_list_lock);

This one looks good to me.

Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

[RFCv0 3/5] Bluetooth: Helper functions for locking change

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>


Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 9a23b19..a7e5a55 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -241,7 +241,7 @@ static char *state_to_string(int state)
 	return "invalid state";
 }
 
-static void l2cap_state_change(struct l2cap_chan *chan, int state)
+static void __l2cap_state_change(struct l2cap_chan *chan, int state)
 {
 	BT_DBG("%p %s -> %s", chan, state_to_string(chan->state),
 						state_to_string(state));
@@ -250,6 +250,27 @@ static void l2cap_state_change(struct l2cap_chan *chan, int state)
 	chan->ops->state_change(chan->data, state);
 }
 
+static void l2cap_state_change(struct l2cap_chan *chan, int state)
+{
+	lock_sock(chan->sk);
+	__l2cap_state_change(chan, state);
+	release_sock(chan->sk);
+}
+
+static inline void __l2cap_set_sock_err(struct sock *sk, int err)
+{
+	sk->sk_err = err;
+}
+
+static inline void l2cap_set_sock_err(struct l2cap_chan *chan, int err)
+{
+	struct sock *sk = chan->sk;
+
+	lock_sock(sk);
+	__l2cap_set_sock_err(sk, err);
+	release_sock(sk);
+}
+
 static void l2cap_chan_timeout(struct work_struct *work)
 {
 	struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
-- 
1.7.4.1

Re: [RFCv0 3/5] Bluetooth: Helper functions for locking change

[Ulisses Furquim]

Hi Andrei,

On Mon, Jan 30, 2012 at 1:09 PM, Emeltchenko Andrei
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

No commit message? In my opinion all commits should have a commit
message and even more in this patch set that deals with locking in
L2CAP core.

> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |   23 ++++++++++++++++++++++-
>  1 files changed, 22 insertions(+), 1 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 9a23b19..a7e5a55 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -241,7 +241,7 @@ static char *state_to_string(int state)
>        return "invalid state";
>  }
>
> -static void l2cap_state_change(struct l2cap_chan *chan, int state)
> +static void __l2cap_state_change(struct l2cap_chan *chan, int state)
>  {
>        BT_DBG("%p %s -> %s", chan, state_to_string(chan->state),
>                                                state_to_string(state));
> @@ -250,6 +250,27 @@ static void l2cap_state_change(struct l2cap_chan *chan, int state)
>        chan->ops->state_change(chan->data, state);
>  }
>
> +static void l2cap_state_change(struct l2cap_chan *chan, int state)
> +{
> +       lock_sock(chan->sk);
> +       __l2cap_state_change(chan, state);
> +       release_sock(chan->sk);
> +}

Introducing this change now before the others in patch 5 will actually
change how l2cap_state_change works. Won't that cause any problems?

> +static inline void __l2cap_set_sock_err(struct sock *sk, int err)
> +{
> +       sk->sk_err = err;
> +}
> +
> +static inline void l2cap_set_sock_err(struct l2cap_chan *chan, int err)
> +{
> +       struct sock *sk = chan->sk;
> +
> +       lock_sock(sk);
> +       __l2cap_set_sock_err(sk, err);
> +       release_sock(sk);
> +}

This helper is different because it didn't exist before so I can say
we shall have no problems with it.

>  static void l2cap_chan_timeout(struct work_struct *work)
>  {
>        struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
> --
> 1.7.4.1

Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

[RFCv0 4/5] Bluetooth: Remove unneeded sk variable

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

In debug use chan %p instead of sk.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index a7e5a55..4a22602 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1589,13 +1589,12 @@ static struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan,
 						struct msghdr *msg, size_t len,
 						u32 priority)
 {
-	struct sock *sk = chan->sk;
 	struct l2cap_conn *conn = chan->conn;
 	struct sk_buff *skb;
 	int err, count, hlen = L2CAP_HDR_SIZE + L2CAP_PSMLEN_SIZE;
 	struct l2cap_hdr *lh;
 
-	BT_DBG("sk %p len %d priority %u", sk, (int)len, priority);
+	BT_DBG("chan %p len %d priority %u", chan, (int)len, priority);
 
 	count = min_t(unsigned int, (conn->mtu - hlen), len);
 
@@ -1625,13 +1624,12 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
 						struct msghdr *msg, size_t len,
 						u32 priority)
 {
-	struct sock *sk = chan->sk;
 	struct l2cap_conn *conn = chan->conn;
 	struct sk_buff *skb;
 	int err, count, hlen = L2CAP_HDR_SIZE;
 	struct l2cap_hdr *lh;
 
-	BT_DBG("sk %p len %d", sk, (int)len);
+	BT_DBG("chan %p len %d", chan, (int)len);
 
 	count = min_t(unsigned int, (conn->mtu - hlen), len);
 
@@ -1660,13 +1658,12 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
 						struct msghdr *msg, size_t len,
 						u32 control, u16 sdulen)
 {
-	struct sock *sk = chan->sk;
 	struct l2cap_conn *conn = chan->conn;
 	struct sk_buff *skb;
 	int err, count, hlen;
 	struct l2cap_hdr *lh;
 
-	BT_DBG("sk %p len %d", sk, (int)len);
+	BT_DBG("chan %p len %d", chan, (int)len);
 
 	if (!conn)
 		return ERR_PTR(-ENOTCONN);
-- 
1.7.4.1

Re: [RFCv0 4/5] Bluetooth: Remove unneeded sk variable

[Ulisses Furquim]

Hi Andrei,

On Mon, Jan 30, 2012 at 1:09 PM, Emeltchenko Andrei
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> In debug use chan %p instead of sk.
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |    9 +++------
>  1 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index a7e5a55..4a22602 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1589,13 +1589,12 @@ static struct sk_buff *l2cap_create_connless_pdu(struct l2cap_chan *chan,
>                                                struct msghdr *msg, size_t len,
>                                                u32 priority)
>  {
> -       struct sock *sk = chan->sk;
>        struct l2cap_conn *conn = chan->conn;
>        struct sk_buff *skb;
>        int err, count, hlen = L2CAP_HDR_SIZE + L2CAP_PSMLEN_SIZE;
>        struct l2cap_hdr *lh;
>
> -       BT_DBG("sk %p len %d priority %u", sk, (int)len, priority);
> +       BT_DBG("chan %p len %d priority %u", chan, (int)len, priority);
>
>        count = min_t(unsigned int, (conn->mtu - hlen), len);
>
> @@ -1625,13 +1624,12 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
>                                                struct msghdr *msg, size_t len,
>                                                u32 priority)
>  {
> -       struct sock *sk = chan->sk;
>        struct l2cap_conn *conn = chan->conn;
>        struct sk_buff *skb;
>        int err, count, hlen = L2CAP_HDR_SIZE;
>        struct l2cap_hdr *lh;
>
> -       BT_DBG("sk %p len %d", sk, (int)len);
> +       BT_DBG("chan %p len %d", chan, (int)len);
>
>        count = min_t(unsigned int, (conn->mtu - hlen), len);
>
> @@ -1660,13 +1658,12 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
>                                                struct msghdr *msg, size_t len,
>                                                u32 control, u16 sdulen)
>  {
> -       struct sock *sk = chan->sk;
>        struct l2cap_conn *conn = chan->conn;
>        struct sk_buff *skb;
>        int err, count, hlen;
>        struct l2cap_hdr *lh;
>
> -       BT_DBG("sk %p len %d", sk, (int)len);
> +       BT_DBG("chan %p len %d", chan, (int)len);
>
>        if (!conn)
>                return ERR_PTR(-ENOTCONN);

This one looks good to me.

Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

[RFCv0 5/5] Bluetooth: Change sk lock to l2cap_chan lock

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Change socket lock to l2cap_chan lock. This is needed for use l2cap
channels without opening kernel socket for locking.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |  220 +++++++++++++++++++++++++++-----------------
 net/bluetooth/l2cap_sock.c |   13 ++-
 2 files changed, 146 insertions(+), 87 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 4a22602..85b4572 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -275,12 +275,12 @@ static void l2cap_chan_timeout(struct work_struct *work)
 {
 	struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
 							chan_timer.work);
-	struct sock *sk = chan->sk;
 	int reason;
 
 	BT_DBG("chan %p state %d", chan, chan->state);
 
-	lock_sock(sk);
+	mutex_lock(&chan->conn->chan_lock);
+	l2cap_chan_lock(chan);
 
 	if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
 		reason = ECONNREFUSED;
@@ -292,7 +292,8 @@ static void l2cap_chan_timeout(struct work_struct *work)
 
 	l2cap_chan_close(chan, reason);
 
-	release_sock(sk);
+	l2cap_chan_unlock(chan);
+	mutex_unlock(&chan->conn->chan_lock);
 
 	chan->ops->close(chan->data);
 	l2cap_chan_put(chan);
@@ -406,11 +407,13 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
 		hci_conn_put(conn->hcon);
 	}
 
-	l2cap_state_change(chan, BT_CLOSED);
+	lock_sock(sk);
+
+	__l2cap_state_change(chan, BT_CLOSED);
 	sock_set_flag(sk, SOCK_ZAPPED);
 
 	if (err)
-		sk->sk_err = err;
+		__l2cap_set_sock_err(sk, err);
 
 	if (parent) {
 		bt_accept_unlink(sk);
@@ -418,6 +421,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;
@@ -449,10 +454,14 @@ static void l2cap_chan_cleanup_listen(struct sock *parent)
 	/* Close not yet accepted channels */
 	while ((sk = bt_accept_dequeue(parent, NULL))) {
 		struct l2cap_chan *chan = l2cap_pi(sk)->chan;
+
+		mutex_lock(&chan->conn->chan_lock);
+		l2cap_chan_lock(chan);
 		__clear_chan_timer(chan);
-		lock_sock(sk);
 		l2cap_chan_close(chan, ECONNRESET);
-		release_sock(sk);
+		l2cap_chan_unlock(chan);
+		mutex_unlock(&chan->conn->chan_lock);
+
 		chan->ops->close(chan->data);
 	}
 }
@@ -466,10 +475,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);
+		__l2cap_state_change(chan, BT_CLOSED);
 		sock_set_flag(sk, SOCK_ZAPPED);
+		release_sock(sk);
 		break;
 
 	case BT_CONNECTED:
@@ -512,7 +523,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;
 	}
 }
@@ -740,14 +753,12 @@ 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;
+	struct sock *sk = chan->sk;
 	struct l2cap_disconn_req req;
 
 	if (!conn)
 		return;
 
-	sk = chan->sk;
-
 	if (chan->mode == L2CAP_MODE_ERTM) {
 		__clear_retrans_timer(chan);
 		__clear_monitor_timer(chan);
@@ -759,8 +770,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);
 
-	l2cap_state_change(chan, BT_DISCONN);
-	sk->sk_err = err;
+	lock_sock(sk);
+	__l2cap_state_change(chan, BT_DISCONN);
+	__l2cap_set_sock_err(sk, err);
+	release_sock(sk);
 }
 
 /* ---- L2CAP connections ---- */
@@ -775,10 +788,10 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 	list_for_each_entry_safe(chan, tmp, &conn->chan_l, list) {
 		struct sock *sk = chan->sk;
 
-		bh_lock_sock(sk);
+		l2cap_chan_lock(chan);
 
 		if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
-			bh_unlock_sock(sk);
+			l2cap_chan_unlock(chan);
 			continue;
 		}
 
@@ -787,7 +800,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 
 			if (!l2cap_chan_check_security(chan) ||
 					!__l2cap_no_conn_pending(chan)) {
-				bh_unlock_sock(sk);
+				l2cap_chan_unlock(chan);
 				continue;
 			}
 
@@ -797,7 +810,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 				/* l2cap_chan_close() calls list_del(chan)
 				 * so release the lock */
 				l2cap_chan_close(chan, ECONNRESET);
-				bh_unlock_sock(sk);
+				l2cap_chan_unlock(chan);
 				continue;
 			}
 
@@ -817,6 +830,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 			rsp.dcid = cpu_to_le16(chan->scid);
 
 			if (l2cap_chan_check_security(chan)) {
+				lock_sock(sk);
 				if (bt_sk(sk)->defer_setup) {
 					struct sock *parent = bt_sk(sk)->parent;
 					rsp.result = cpu_to_le16(L2CAP_CR_PEND);
@@ -825,10 +839,11 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 						parent->sk_data_ready(parent, 0);
 
 				} else {
-					l2cap_state_change(chan, BT_CONFIG);
+					__l2cap_state_change(chan, BT_CONFIG);
 					rsp.result = cpu_to_le16(L2CAP_CR_SUCCESS);
 					rsp.status = cpu_to_le16(L2CAP_CS_NO_INFO);
 				}
+				release_sock(sk);
 			} else {
 				rsp.result = cpu_to_le16(L2CAP_CR_PEND);
 				rsp.status = cpu_to_le16(L2CAP_CS_AUTHEN_PEND);
@@ -839,7 +854,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 
 			if (test_bit(CONF_REQ_SENT, &chan->conf_state) ||
 					rsp.result != L2CAP_CR_SUCCESS) {
-				bh_unlock_sock(sk);
+				l2cap_chan_unlock(chan);
 				continue;
 			}
 
@@ -849,7 +864,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 			chan->num_conf_req++;
 		}
 
-		bh_unlock_sock(sk);
+		l2cap_chan_unlock(chan);
 	}
 
 	mutex_unlock(&conn->chan_lock);
@@ -928,7 +943,7 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 
 	__set_chan_timer(chan, sk->sk_sndtimeo);
 
-	l2cap_state_change(chan, BT_CONNECTED);
+	__l2cap_state_change(chan, BT_CONNECTED);
 	parent->sk_data_ready(parent, 0);
 
 clean:
@@ -938,18 +953,24 @@ clean:
 static void l2cap_chan_ready(struct l2cap_chan *chan)
 {
 	struct sock *sk = chan->sk;
-	struct sock *parent = bt_sk(sk)->parent;
+	struct sock *parent;
+
+	lock_sock(sk);
+
+	parent = bt_sk(sk)->parent;
 
 	BT_DBG("sk %p, parent %p", sk, parent);
 
 	chan->conf_state = 0;
 	__clear_chan_timer(chan);
 
-	l2cap_state_change(chan, BT_CONNECTED);
+	__l2cap_state_change(chan, BT_CONNECTED);
 	sk->sk_state_change(sk);
 
 	if (parent)
 		parent->sk_data_ready(parent, 0);
+
+	release_sock(sk);
 }
 
 static void l2cap_conn_ready(struct l2cap_conn *conn)
@@ -964,29 +985,31 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
 	if (conn->hcon->out && conn->hcon->type == LE_LINK)
 		smp_conn_security(conn, conn->hcon->pending_sec_level);
 
-	rcu_read_lock();
+	mutex_lock(&conn->chan_lock);
 
-	list_for_each_entry_rcu(chan, &conn->chan_l, list) {
-		struct sock *sk = chan->sk;
+	list_for_each_entry(chan, &conn->chan_l, list) {
 
-		bh_lock_sock(sk);
+		l2cap_chan_lock(chan);
 
 		if (conn->hcon->type == LE_LINK) {
 			if (smp_conn_security(conn, chan->sec_level))
 				l2cap_chan_ready(chan);
 
 		} else if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
+			struct sock *sk = chan->sk;
 			__clear_chan_timer(chan);
-			l2cap_state_change(chan, BT_CONNECTED);
+			lock_sock(sk);
+			__l2cap_state_change(chan, BT_CONNECTED);
 			sk->sk_state_change(sk);
+			release_sock(sk);
 
 		} else if (chan->state == BT_CONNECT)
 			l2cap_do_start(chan);
 
-		bh_unlock_sock(sk);
+		l2cap_chan_unlock(chan);
 	}
 
-	rcu_read_unlock();
+	mutex_unlock(&conn->chan_lock);
 }
 
 /* Notify sockets that we cannot guaranty reliability anymore */
@@ -1001,8 +1024,12 @@ static void l2cap_conn_unreliable(struct l2cap_conn *conn, int err)
 	list_for_each_entry_rcu(chan, &conn->chan_l, list) {
 		struct sock *sk = chan->sk;
 
+		lock_sock(sk);
+
 		if (test_bit(FLAG_FORCE_RELIABLE, &chan->flags))
-			sk->sk_err = err;
+			__l2cap_set_sock_err(sk, err);
+
+		release_sock(sk);
 	}
 
 	rcu_read_unlock();
@@ -1023,7 +1050,6 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 {
 	struct l2cap_conn *conn = hcon->l2cap_data;
 	struct l2cap_chan *chan, *l;
-	struct sock *sk;
 
 	if (!conn)
 		return;
@@ -1036,10 +1062,12 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 
 	/* Kill channels */
 	list_for_each_entry_safe(chan, l, &conn->chan_l, list) {
-		sk = chan->sk;
-		lock_sock(sk);
+		l2cap_chan_lock(chan);
+
 		l2cap_chan_del(chan, err);
-		release_sock(sk);
+
+		l2cap_chan_unlock(chan);
+
 		chan->ops->close(chan->data);
 	}
 
@@ -1251,14 +1279,14 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
 
 	l2cap_chan_add(conn, chan);
 
-	l2cap_state_change(chan, BT_CONNECT);
+	__l2cap_state_change(chan, BT_CONNECT);
 	__set_chan_timer(chan, sk->sk_sndtimeo);
 
 	if (hcon->state == BT_CONNECTED) {
 		if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
 			__clear_chan_timer(chan);
 			if (l2cap_chan_check_security(chan))
-				l2cap_state_change(chan, BT_CONNECTED);
+				__l2cap_state_change(chan, BT_CONNECTED);
 		} else
 			l2cap_do_start(chan);
 	}
@@ -1307,40 +1335,39 @@ static void l2cap_monitor_timeout(struct work_struct *work)
 {
 	struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
 							monitor_timer.work);
-	struct sock *sk = chan->sk;
-
 	BT_DBG("chan %p", chan);
 
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
+
 	if (chan->retry_count >= chan->remote_max_tx) {
 		l2cap_send_disconn_req(chan->conn, chan, ECONNABORTED);
-		release_sock(sk);
-		return;
+		goto done;
 	}
 
 	chan->retry_count++;
 	__set_monitor_timer(chan);
 
 	l2cap_send_rr_or_rnr(chan, L2CAP_CTRL_POLL);
-	release_sock(sk);
+done:
+	l2cap_chan_unlock(chan);
 }
 
 static void l2cap_retrans_timeout(struct work_struct *work)
 {
 	struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
 							retrans_timer.work);
-	struct sock *sk = chan->sk;
-
 	BT_DBG("chan %p", chan);
 
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
+
 	chan->retry_count = 1;
 	__set_monitor_timer(chan);
 
 	set_bit(CONN_WAIT_F, &chan->conn_state);
 
 	l2cap_send_rr_or_rnr(chan, L2CAP_CTRL_POLL);
-	release_sock(sk);
+
+	l2cap_chan_unlock(chan);
 }
 
 static void l2cap_drop_acked_frames(struct l2cap_chan *chan)
@@ -2029,9 +2056,11 @@ static void l2cap_ack_timeout(struct work_struct *work)
 
 	BT_DBG("chan %p", chan);
 
-	lock_sock(chan->sk);
+	l2cap_chan_lock(chan);
+
 	__l2cap_send_ack(chan);
-	release_sock(chan->sk);
+
+	l2cap_chan_unlock(chan);
 
 	l2cap_chan_put(chan);
 }
@@ -2702,22 +2731,22 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_DONE) {
 		if (l2cap_chan_check_security(chan)) {
 			if (bt_sk(sk)->defer_setup) {
-				l2cap_state_change(chan, BT_CONNECT2);
+				__l2cap_state_change(chan, BT_CONNECT2);
 				result = L2CAP_CR_PEND;
 				status = L2CAP_CS_AUTHOR_PEND;
 				parent->sk_data_ready(parent, 0);
 			} else {
-				l2cap_state_change(chan, BT_CONFIG);
+				__l2cap_state_change(chan, BT_CONFIG);
 				result = L2CAP_CR_SUCCESS;
 				status = L2CAP_CS_NO_INFO;
 			}
 		} else {
-			l2cap_state_change(chan, BT_CONNECT2);
+			__l2cap_state_change(chan, BT_CONNECT2);
 			result = L2CAP_CR_PEND;
 			status = L2CAP_CS_AUTHEN_PEND;
 		}
 	} else {
-		l2cap_state_change(chan, BT_CONNECT2);
+		__l2cap_state_change(chan, BT_CONNECT2);
 		result = L2CAP_CR_PEND;
 		status = L2CAP_CS_NO_INFO;
 	}
@@ -2774,15 +2803,18 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	BT_DBG("dcid 0x%4.4x scid 0x%4.4x result 0x%2.2x status 0x%2.2x", dcid, scid, result, status);
 
 	if (scid) {
-		chan = l2cap_get_chan_by_scid(conn, scid);
+		chan = __l2cap_get_chan_by_scid(conn, scid);
 		if (!chan)
 			return -EFAULT;
 	} else {
-		chan = l2cap_get_chan_by_ident(conn, cmd->ident);
+		chan = __l2cap_get_chan_by_ident(conn, cmd->ident);
 		if (!chan)
 			return -EFAULT;
 	}
 
+	mutex_lock(&conn->chan_lock);
+	l2cap_chan_lock(chan);
+
 	sk = chan->sk;
 
 	switch (result) {
@@ -2809,7 +2841,9 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 		break;
 	}
 
-	release_sock(sk);
+	l2cap_chan_unlock(chan);
+	mutex_unlock(&conn->chan_lock);
+
 	return 0;
 }
 
@@ -2838,10 +2872,12 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 
 	BT_DBG("dcid 0x%4.4x flags 0x%2.2x", dcid, flags);
 
-	chan = l2cap_get_chan_by_scid(conn, dcid);
+	chan = __l2cap_get_chan_by_scid(conn, dcid);
 	if (!chan)
 		return -ENOENT;
 
+	l2cap_chan_lock(chan);
+
 	sk = chan->sk;
 
 	if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
@@ -2931,7 +2967,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	}
 
 unlock:
-	release_sock(sk);
+	l2cap_chan_unlock(chan);
 	return 0;
 }
 
@@ -2950,10 +2986,12 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	BT_DBG("scid 0x%4.4x flags 0x%2.2x result 0x%2.2x",
 			scid, flags, result);
 
-	chan = l2cap_get_chan_by_scid(conn, scid);
+	chan = __l2cap_get_chan_by_scid(conn, scid);
 	if (!chan)
 		return 0;
 
+	l2cap_chan_lock(chan);
+
 	sk = chan->sk;
 
 	switch (result) {
@@ -3013,7 +3051,8 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 		}
 
 	default:
-		sk->sk_err = ECONNRESET;
+		l2cap_set_sock_err(chan, ECONNRESET);
+
 		__set_chan_timer(chan,
 				msecs_to_jiffies(L2CAP_DISC_REJ_TIMEOUT));
 		l2cap_send_disconn_req(conn, chan, ECONNRESET);
@@ -3039,7 +3078,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	}
 
 done:
-	release_sock(sk);
+	l2cap_chan_unlock(chan);
 	return 0;
 }
 
@@ -3056,20 +3095,27 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
 
 	BT_DBG("scid 0x%4.4x dcid 0x%4.4x", scid, dcid);
 
-	chan = l2cap_get_chan_by_scid(conn, dcid);
+	chan = __l2cap_get_chan_by_scid(conn, dcid);
 	if (!chan)
 		return 0;
 
+	mutex_lock(&conn->chan_lock);
+	l2cap_chan_lock(chan);
+
 	sk = chan->sk;
 
 	rsp.dcid = cpu_to_le16(chan->scid);
 	rsp.scid = cpu_to_le16(chan->dcid);
 	l2cap_send_cmd(conn, cmd->ident, L2CAP_DISCONN_RSP, sizeof(rsp), &rsp);
 
+	lock_sock(sk);
 	sk->sk_shutdown = SHUTDOWN_MASK;
+	release_sock(sk);
 
 	l2cap_chan_del(chan, ECONNRESET);
-	release_sock(sk);
+
+	l2cap_chan_unlock(chan);
+	mutex_unlock(&conn->chan_lock);
 
 	chan->ops->close(chan->data);
 	return 0;
@@ -3080,21 +3126,23 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
 	struct l2cap_disconn_rsp *rsp = (struct l2cap_disconn_rsp *) data;
 	u16 dcid, scid;
 	struct l2cap_chan *chan;
-	struct sock *sk;
 
 	scid = __le16_to_cpu(rsp->scid);
 	dcid = __le16_to_cpu(rsp->dcid);
 
 	BT_DBG("dcid 0x%4.4x scid 0x%4.4x", dcid, scid);
 
-	chan = l2cap_get_chan_by_scid(conn, scid);
+	chan = __l2cap_get_chan_by_scid(conn, scid);
 	if (!chan)
 		return 0;
 
-	sk = chan->sk;
+	mutex_lock(&conn->chan_lock);
+	l2cap_chan_lock(chan);
 
 	l2cap_chan_del(chan, 0);
-	release_sock(sk);
+
+	l2cap_chan_unlock(chan);
+	mutex_unlock(&conn->chan_lock);
 
 	chan->ops->close(chan->data);
 	return 0;
@@ -4243,18 +4291,17 @@ drop:
 static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk_buff *skb)
 {
 	struct l2cap_chan *chan;
-	struct sock *sk = NULL;
 	u32 control;
 	u16 tx_seq;
 	int len;
 
-	chan = l2cap_get_chan_by_scid(conn, cid);
+	chan = __l2cap_get_chan_by_scid(conn, cid);
 	if (!chan) {
 		BT_DBG("unknown cid 0x%4.4x", cid);
 		goto drop;
 	}
 
-	sk = chan->sk;
+	l2cap_chan_lock(chan);
 
 	BT_DBG("chan %p, len %d", chan, skb->len);
 
@@ -4325,8 +4372,7 @@ drop:
 	kfree_skb(skb);
 
 done:
-	if (sk)
-		release_sock(sk);
+	l2cap_chan_unlock(chan);
 
 	return 0;
 }
@@ -4542,12 +4588,10 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 		__cancel_delayed_work(&conn->security_timer);
 	}
 
-	rcu_read_lock();
-
-	list_for_each_entry_rcu(chan, &conn->chan_l, list) {
-		struct sock *sk = chan->sk;
+	mutex_lock(&conn->chan_lock);
 
-		bh_lock_sock(sk);
+	list_for_each_entry(chan, &conn->chan_l, list) {
+		l2cap_chan_lock(chan);
 
 		BT_DBG("chan->scid %d", chan->scid);
 
@@ -4557,19 +4601,19 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 				l2cap_chan_ready(chan);
 			}
 
-			bh_unlock_sock(sk);
+			l2cap_chan_unlock(chan);
 			continue;
 		}
 
 		if (test_bit(CONF_CONNECT_PEND, &chan->conf_state)) {
-			bh_unlock_sock(sk);
+			l2cap_chan_unlock(chan);
 			continue;
 		}
 
 		if (!status && (chan->state == BT_CONNECTED ||
 						chan->state == BT_CONFIG)) {
 			l2cap_check_encryption(chan, encrypt);
-			bh_unlock_sock(sk);
+			l2cap_chan_unlock(chan);
 			continue;
 		}
 
@@ -4590,9 +4634,12 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 					msecs_to_jiffies(L2CAP_DISC_TIMEOUT));
 			}
 		} else if (chan->state == BT_CONNECT2) {
+			struct sock *sk = chan->sk;
 			struct l2cap_conn_rsp rsp;
 			__u16 res, stat;
 
+			lock_sock(sk);
+
 			if (!status) {
 				if (bt_sk(sk)->defer_setup) {
 					struct sock *parent = bt_sk(sk)->parent;
@@ -4601,18 +4648,20 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 					if (parent)
 						parent->sk_data_ready(parent, 0);
 				} else {
-					l2cap_state_change(chan, BT_CONFIG);
+					__l2cap_state_change(chan, BT_CONFIG);
 					res = L2CAP_CR_SUCCESS;
 					stat = L2CAP_CS_NO_INFO;
 				}
 			} else {
-				l2cap_state_change(chan, BT_DISCONN);
+				__l2cap_state_change(chan, BT_DISCONN);
 				__set_chan_timer(chan,
 					msecs_to_jiffies(L2CAP_DISC_TIMEOUT));
 				res = L2CAP_CR_SEC_BLOCK;
 				stat = L2CAP_CS_NO_INFO;
 			}
 
+			release_sock(sk);
+
 			rsp.scid   = cpu_to_le16(chan->dcid);
 			rsp.dcid   = cpu_to_le16(chan->scid);
 			rsp.result = cpu_to_le16(res);
@@ -4621,10 +4670,10 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 							sizeof(rsp), &rsp);
 		}
 
-		bh_unlock_sock(sk);
+		l2cap_chan_unlock(chan);
 	}
 
-	rcu_read_unlock();
+	mutex_unlock(&conn->chan_lock);
 
 	return 0;
 }
@@ -4681,10 +4730,11 @@ int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
 			goto drop;
 		}
 
-		chan = l2cap_get_chan_by_scid(conn, cid);
+		chan = __l2cap_get_chan_by_scid(conn, cid);
 
 		if (chan && chan->sk) {
 			struct sock *sk = chan->sk;
+			lock_sock(sk);
 
 			if (chan->imtu < len - L2CAP_HDR_SIZE) {
 				BT_ERR("Frame exceeding recv MTU (len %d, "
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 1636029..dea5418 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -809,7 +809,9 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 			err = __l2cap_wait_ack(sk);
 
 		sk->sk_shutdown = SHUTDOWN_MASK;
+		release_sock(sk);
 		l2cap_chan_close(chan, 0);
+		lock_sock(sk);
 
 		if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime)
 			err = bt_sock_wait_state(sk, BT_CLOSED,
@@ -862,8 +864,12 @@ static int l2cap_sock_recv_cb(void *data, struct sk_buff *skb)
 	struct sock *sk = data;
 	struct l2cap_pinfo *pi = l2cap_pi(sk);
 
-	if (pi->rx_busy_skb)
-		return -ENOMEM;
+	lock_sock(sk);
+
+	if (pi->rx_busy_skb) {
+		err = -ENOMEM;
+		goto done;
+	}
 
 	err = sock_queue_rcv_skb(sk, skb);
 
@@ -882,6 +888,9 @@ static int l2cap_sock_recv_cb(void *data, struct sk_buff *skb)
 		err = 0;
 	}
 
+done:
+	release_sock(sk);
+
 	return err;
 }
 
-- 
1.7.4.1

Re: [RFCv0 5/5] Bluetooth: Change sk lock to l2cap_chan lock

[Ulisses Furquim]

Hi Andrei,

On Mon, Jan 30, 2012 at 1:09 PM, Emeltchenko Andrei
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Change socket lock to l2cap_chan lock. This is needed for use l2cap
> channels without opening kernel socket for locking.
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |  220 +++++++++++++++++++++++++++-----------------
>  net/bluetooth/l2cap_sock.c |   13 ++-
>  2 files changed, 146 insertions(+), 87 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 4a22602..85b4572 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -275,12 +275,12 @@ static void l2cap_chan_timeout(struct work_struct *work)
>  {
>        struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
>                                                        chan_timer.work);
> -       struct sock *sk = chan->sk;
>        int reason;
>
>        BT_DBG("chan %p state %d", chan, chan->state);
>
> -       lock_sock(sk);
> +       mutex_lock(&chan->conn->chan_lock);
> +       l2cap_chan_lock(chan);

Ugh, this doesn't look right or even pretty. Why do we need it this way?

>        if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
>                reason = ECONNREFUSED;
> @@ -292,7 +292,8 @@ static void l2cap_chan_timeout(struct work_struct *work)
>
>        l2cap_chan_close(chan, reason);
>
> -       release_sock(sk);
> +       l2cap_chan_unlock(chan);
> +       mutex_unlock(&chan->conn->chan_lock);
>
>        chan->ops->close(chan->data);
>        l2cap_chan_put(chan);
> @@ -406,11 +407,13 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
>                hci_conn_put(conn->hcon);
>        }
>
> -       l2cap_state_change(chan, BT_CLOSED);
> +       lock_sock(sk);
> +
> +       __l2cap_state_change(chan, BT_CLOSED);
>        sock_set_flag(sk, SOCK_ZAPPED);
>
>        if (err)
> -               sk->sk_err = err;
> +               __l2cap_set_sock_err(sk, err);
>
>        if (parent) {
>                bt_accept_unlink(sk);
> @@ -418,6 +421,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;
> @@ -449,10 +454,14 @@ static void l2cap_chan_cleanup_listen(struct sock *parent)
>        /* Close not yet accepted channels */
>        while ((sk = bt_accept_dequeue(parent, NULL))) {
>                struct l2cap_chan *chan = l2cap_pi(sk)->chan;
> +
> +               mutex_lock(&chan->conn->chan_lock);
> +               l2cap_chan_lock(chan);

Again.

>                __clear_chan_timer(chan);
> -               lock_sock(sk);
>                l2cap_chan_close(chan, ECONNRESET);
> -               release_sock(sk);
> +               l2cap_chan_unlock(chan);
> +               mutex_unlock(&chan->conn->chan_lock);
> +
>                chan->ops->close(chan->data);
>        }
>  }

<snip>

> @@ -964,29 +985,31 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
>        if (conn->hcon->out && conn->hcon->type == LE_LINK)
>                smp_conn_security(conn, conn->hcon->pending_sec_level);
>
> -       rcu_read_lock();
> +       mutex_lock(&conn->chan_lock);
>
> -       list_for_each_entry_rcu(chan, &conn->chan_l, list) {
> -               struct sock *sk = chan->sk;
> +       list_for_each_entry(chan, &conn->chan_l, list) {

Why are you removing RCU read locks here?

> -               bh_lock_sock(sk);
> +               l2cap_chan_lock(chan);
>
>                if (conn->hcon->type == LE_LINK) {
>                        if (smp_conn_security(conn, chan->sec_level))
>                                l2cap_chan_ready(chan);
>
>                } else if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
> +                       struct sock *sk = chan->sk;
>                        __clear_chan_timer(chan);
> -                       l2cap_state_change(chan, BT_CONNECTED);
> +                       lock_sock(sk);
> +                       __l2cap_state_change(chan, BT_CONNECTED);
>                        sk->sk_state_change(sk);
> +                       release_sock(sk);
>
>                } else if (chan->state == BT_CONNECT)
>                        l2cap_do_start(chan);
>
> -               bh_unlock_sock(sk);
> +               l2cap_chan_unlock(chan);
>        }
>
> -       rcu_read_unlock();
> +       mutex_unlock(&conn->chan_lock);
>  }

<snip>

This patch still mixes the return of using conn->chan_lock with
locking of l2cap_chan. It should be possible and better to have these
changes in different patches. Another question is will you remove RCU
usage for conn->chan_l completely or not?

Thanks,
Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [RFCv0 5/5] Bluetooth: Change sk lock to l2cap_chan lock

[Emeltchenko Andrei]

Hi Ulisses,

On Mon, Jan 30, 2012 at 03:46:27PM -0200, Ulisses Furquim wrote:
> Hi Andrei,
> 
> On Mon, Jan 30, 2012 at 1:09 PM, Emeltchenko Andrei
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > Change socket lock to l2cap_chan lock. This is needed for use l2cap
> > channels without opening kernel socket for locking.
> >
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  net/bluetooth/l2cap_core.c |  220 +++++++++++++++++++++++++++-----------------
> >  net/bluetooth/l2cap_sock.c |   13 ++-
> >  2 files changed, 146 insertions(+), 87 deletions(-)
> >
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index 4a22602..85b4572 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -275,12 +275,12 @@ static void l2cap_chan_timeout(struct work_struct *work)
> >  {
> >        struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
> >                                                        chan_timer.work);
> > -       struct sock *sk = chan->sk;
> >        int reason;
> >
> >        BT_DBG("chan %p state %d", chan, chan->state);
> >
> > -       lock_sock(sk);
> > +       mutex_lock(&chan->conn->chan_lock);
> > +       l2cap_chan_lock(chan);
> 
> Ugh, this doesn't look right or even pretty. Why do we need it this way?

I try to keep order of locking, first conn->chan_lock and then chan->lock
otherwise I get warnings from lockdep. I am open to suggestions how to
make it better.

> >        if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
> >                reason = ECONNREFUSED;
> > @@ -292,7 +292,8 @@ static void l2cap_chan_timeout(struct work_struct *work)
> >
> >        l2cap_chan_close(chan, reason);
> >
> > -       release_sock(sk);
> > +       l2cap_chan_unlock(chan);
> > +       mutex_unlock(&chan->conn->chan_lock);
> >
> >        chan->ops->close(chan->data);
> >        l2cap_chan_put(chan);
> > @@ -406,11 +407,13 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
> >                hci_conn_put(conn->hcon);
> >        }
> >
> > -       l2cap_state_change(chan, BT_CLOSED);
> > +       lock_sock(sk);
> > +
> > +       __l2cap_state_change(chan, BT_CLOSED);
> >        sock_set_flag(sk, SOCK_ZAPPED);
> >
> >        if (err)
> > -               sk->sk_err = err;
> > +               __l2cap_set_sock_err(sk, err);
> >
> >        if (parent) {
> >                bt_accept_unlink(sk);
> > @@ -418,6 +421,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;
> > @@ -449,10 +454,14 @@ static void l2cap_chan_cleanup_listen(struct sock *parent)
> >        /* Close not yet accepted channels */
> >        while ((sk = bt_accept_dequeue(parent, NULL))) {
> >                struct l2cap_chan *chan = l2cap_pi(sk)->chan;
> > +
> > +               mutex_lock(&chan->conn->chan_lock);
> > +               l2cap_chan_lock(chan);
> 
> Again.
> 
> >                __clear_chan_timer(chan);
> > -               lock_sock(sk);
> >                l2cap_chan_close(chan, ECONNRESET);
> > -               release_sock(sk);
> > +               l2cap_chan_unlock(chan);
> > +               mutex_unlock(&chan->conn->chan_lock);
> > +
> >                chan->ops->close(chan->data);
> >        }
> >  }
> 
> <snip>
> 
> > @@ -964,29 +985,31 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
> >        if (conn->hcon->out && conn->hcon->type == LE_LINK)
> >                smp_conn_security(conn, conn->hcon->pending_sec_level);
> >
> > -       rcu_read_lock();
> > +       mutex_lock(&conn->chan_lock);
> >
> > -       list_for_each_entry_rcu(chan, &conn->chan_l, list) {
> > -               struct sock *sk = chan->sk;
> > +       list_for_each_entry(chan, &conn->chan_l, list) {
> 
> Why are you removing RCU read locks here?

Because I use mutexes in l2cap_chan_lock. So I can sleep which is not
allowed inside rcu_read_lock/unlock.

> > -               bh_lock_sock(sk);
> > +               l2cap_chan_lock(chan);
> >
> >                if (conn->hcon->type == LE_LINK) {
> >                        if (smp_conn_security(conn, chan->sec_level))
> >                                l2cap_chan_ready(chan);
> >
> >                } else if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
> > +                       struct sock *sk = chan->sk;
> >                        __clear_chan_timer(chan);
> > -                       l2cap_state_change(chan, BT_CONNECTED);
> > +                       lock_sock(sk);
> > +                       __l2cap_state_change(chan, BT_CONNECTED);
> >                        sk->sk_state_change(sk);
> > +                       release_sock(sk);
> >
> >                } else if (chan->state == BT_CONNECT)
> >                        l2cap_do_start(chan);
> >
> > -               bh_unlock_sock(sk);
> > +               l2cap_chan_unlock(chan);
> >        }
> >
> > -       rcu_read_unlock();
> > +       mutex_unlock(&conn->chan_lock);
> >  }
> 
> <snip>
> 
> This patch still mixes the return of using conn->chan_lock with
> locking of l2cap_chan. It should be possible and better to have these
> changes in different patches. Another question is will you remove RCU
> usage for conn->chan_l completely or not?

As I said the change is only to updaters and to the places where I need to
sleep.

Best regards 
Andrei Emeltchenko