[PATCH 1/3] bluetooth: add write_lock_bh to __l2cap_chan_link

[PATCH 1/3] bluetooth: add write_lock_bh to __l2cap_chan_link

[Gustavo F. Padovan]

fix race condition

Signed-off-by: Gustavo F. Padovan <gustavo@las.ic.unicamp.br>
---
 net/bluetooth/l2cap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index ca4d3b4..355f375 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -175,12 +175,14 @@ static inline void __l2cap_chan_link(struct l2cap_chan_list *l, struct sock *sk)
 {
 	sock_hold(sk);
 
+	write_lock_bh(&l->lock);
 	if (l->head)
 		l2cap_pi(l->head)->prev_c = sk;
 
 	l2cap_pi(sk)->next_c = l->head;
 	l2cap_pi(sk)->prev_c = NULL;
 	l->head = sk;
+	write_unlock_bh(&l->lock);
 }
 
 static inline void l2cap_chan_unlink(struct l2cap_chan_list *l, struct sock *sk)
-- 
1.6.0.6

[PATCH 2/3] bluetooth: add macros for cid channel numbers

[Gustavo F. Padovan]

Signed-off-by: Gustavo F. Padovan <gustavo@las.ic.unicamp.br>
---
 include/net/bluetooth/l2cap.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index f566aa1..e7bf331 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -118,6 +118,13 @@ struct l2cap_conn_rsp {
 #define L2CAP_CS_AUTHEN_PEND  0x0001
 #define L2CAP_CS_AUTHOR_PEND  0x0002
 
+
+/* cid channel type */
+#define L2CAP_CID_SIGNALING	0x0001
+#define L2CAP_CID_CONN_LESS	0x0002
+#define L2CAP_CID_DYN_START	0x0040
+#define L2CAP_CID_DYN_END	0xffff
+
 struct l2cap_conf_req {
 	__le16     dcid;
 	__le16     flags;
-- 
1.6.0.6

[PATCH 3/3] bluetooth: use L2CAP_CID_* macros

[Gustavo F. Padovan]

Use macros instead of hardcoded numbers

Signed-off-by: Gustavo F. Padovan <gustavo@las.ic.unicamp.br>
---
 net/bluetooth/l2cap.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 355f375..6181fc8 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -161,9 +161,9 @@ static inline struct sock *l2cap_get_chan_by_ident(struct l2cap_chan_list *l, u8
 
 static u16 l2cap_alloc_cid(struct l2cap_chan_list *l)
 {
-	u16 cid = 0x0040;
+	u16 cid = L2CAP_CID_DYN_START;
 
-	for (; cid < 0xffff; cid++) {
+	for (; cid < L2CAP_CID_DYN_END; cid++) {
 		if(!__l2cap_get_chan_by_scid(l, cid))
 			return cid;
 	}
@@ -217,13 +217,13 @@ static void __l2cap_chan_add(struct l2cap_conn *conn, struct sock *sk, struct so
 		l2cap_pi(sk)->scid = l2cap_alloc_cid(l);
 	} else if (sk->sk_type == SOCK_DGRAM) {
 		/* Connectionless socket */
-		l2cap_pi(sk)->scid = 0x0002;
-		l2cap_pi(sk)->dcid = 0x0002;
+		l2cap_pi(sk)->scid = L2CAP_CID_CONN_LESS;
+		l2cap_pi(sk)->dcid = L2CAP_CID_CONN_LESS;
 		l2cap_pi(sk)->omtu = L2CAP_DEFAULT_MTU;
 	} else {
 		/* Raw socket can send/recv signalling messages only */
-		l2cap_pi(sk)->scid = 0x0001;
-		l2cap_pi(sk)->dcid = 0x0001;
+		l2cap_pi(sk)->scid = L2CAP_CID_SIGNALING;
+		l2cap_pi(sk)->dcid = L2CAP_CID_SIGNALING;
 		l2cap_pi(sk)->omtu = L2CAP_DEFAULT_MTU;
 	}
 
@@ -1600,7 +1600,7 @@ static struct sk_buff *l2cap_build_cmd(struct l2cap_conn *conn,
 
 	lh = (struct l2cap_hdr *) skb_put(skb, L2CAP_HDR_SIZE);
 	lh->len = cpu_to_le16(L2CAP_CMD_HDR_SIZE + dlen);
-	lh->cid = cpu_to_le16(0x0001);
+	lh->cid = cpu_to_le16(L2CAP_CID_SIGNALING);
 
 	cmd = (struct l2cap_cmd_hdr *) skb_put(skb, L2CAP_CMD_HDR_SIZE);
 	cmd->code  = code;
@@ -2422,11 +2422,11 @@ static void l2cap_recv_frame(struct l2cap_conn *conn, struct sk_buff *skb)
 	BT_DBG("len %d, cid 0x%4.4x", len, cid);
 
 	switch (cid) {
-	case 0x0001:
+	case L2CAP_CID_SIGNALING:
 		l2cap_sig_channel(conn, skb);
 		break;
 
-	case 0x0002:
+	case L2CAP_CID_CONN_LESS:
 		psm = get_unaligned((__le16 *) skb->data);
 		skb_pull(skb, 2);
 		l2cap_conless_channel(conn, psm, skb);
-- 
1.6.0.6

Re: [PATCH 3/3] bluetooth: use L2CAP_CID_* macros

[Marcel Holtmann]

Hi Gustavo,

> Use macros instead of hardcoded numbers

combine this with your previous patch and get me a proper commit
message.

Regards

Marcel

Re: [PATCH 2/3] bluetooth: add macros for cid channel numbers

[Marcel Holtmann]

Hi Gustavo,

you can keep Bluetooth patches on linux-bluetooth only since they have
to go via bluetooth-testing.git first.

You are missing a commit message before the signed-off-by line. I am
serious about that every kernel patch has a proper commit message in
human readable text. No matter how simple the patch might look.

> Signed-off-by: Gustavo F. Padovan <gustavo@las.ic.unicamp.br>
> ---
>  include/net/bluetooth/l2cap.h |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index f566aa1..e7bf331 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -118,6 +118,13 @@ struct l2cap_conn_rsp {
>  #define L2CAP_CS_AUTHEN_PEND  0x0001
>  #define L2CAP_CS_AUTHOR_PEND  0x0002
>  
> +
> +/* cid channel type */

Use /* channel identifier */ here.

> +#define L2CAP_CID_SIGNALING	0x0001
> +#define L2CAP_CID_CONN_LESS	0x0002
> +#define L2CAP_CID_DYN_START	0x0040
> +#define L2CAP_CID_DYN_END	0xffff
> +

And put these above /* connect result */ defs.

Regards

Marcel

Re: [PATCH 1/3] bluetooth: add write_lock_bh to __l2cap_chan_link

[Gustavo F. Padovan]

On Sat, Feb 28, 2009 at 2:02 AM, Gustavo F. Padovan
<gustavo@las.ic.unicamp.br> wrote:
> fix race condition
>
> Signed-off-by: Gustavo F. Padovan <gustavo@las.ic.unicamp.br>
> ---
>  net/bluetooth/l2cap.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index ca4d3b4..355f375 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -175,12 +175,14 @@ static inline void __l2cap_chan_link(struct l2cap_chan_list *l, struct sock *sk)
>  {
>        sock_hold(sk);
>
> +       write_lock_bh(&l->lock);
>        if (l->head)
>                l2cap_pi(l->head)->prev_c = sk;
>
>        l2cap_pi(sk)->next_c = l->head;
>        l2cap_pi(sk)->prev_c = NULL;
>        l->head = sk;
> +       write_unlock_bh(&l->lock);
>  }
>
>  static inline void l2cap_chan_unlink(struct l2cap_chan_list *l, struct sock *sk)



This patch is wrong, ignore it.


> --
> 1.6.0.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Gustavo F. Padovan

Computer Engineering Student
Institute of Computing - IC
University of Campinas - UNICAMP

email: gustavo@las.ic.unicamp.br
gtalk: gfpadovan@gmail.com
mobile: +55 19 81030803

Re: [PATCH 1/3] bluetooth: add write_lock_bh to __l2cap_chan_link

[Marcel Holtmann]

Hi Gustavo,

> fix race condition
> 
> Signed-off-by: Gustavo F. Padovan <gustavo@las.ic.unicamp.br>
> ---
>  net/bluetooth/l2cap.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index ca4d3b4..355f375 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -175,12 +175,14 @@ static inline void __l2cap_chan_link(struct l2cap_chan_list *l, struct sock *sk)
>  {
>  	sock_hold(sk);
>  
> +	write_lock_bh(&l->lock);
>  	if (l->head)
>  		l2cap_pi(l->head)->prev_c = sk;
>  
>  	l2cap_pi(sk)->next_c = l->head;
>  	l2cap_pi(sk)->prev_c = NULL;
>  	l->head = sk;
> +	write_unlock_bh(&l->lock);
>  }

a general rule of thumb is that function starting with __ are the ones
that don't use any kind of locking.

Regards

Marcel

Re: [PATCH 1/3] bluetooth: add write_lock_bh to __l2cap_chan_link

[Gustavo F. Padovan]

On Sat, Feb 28, 2009 at 6:33 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Gustavo,
>
>> fix race condition
>>
>> Signed-off-by: Gustavo F. Padovan <gustavo@las.ic.unicamp.br>
>> ---
>> =A0net/bluetooth/l2cap.c | =A0 =A02 ++
>> =A01 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>> index ca4d3b4..355f375 100644
>> --- a/net/bluetooth/l2cap.c
>> +++ b/net/bluetooth/l2cap.c
>> @@ -175,12 +175,14 @@ static inline void __l2cap_chan_link(struct l2cap_=
chan_list *l, struct sock *sk)
>> =A0{
>> =A0 =A0 =A0 sock_hold(sk);
>>
>> + =A0 =A0 write_lock_bh(&l->lock);
>> =A0 =A0 =A0 if (l->head)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 l2cap_pi(l->head)->prev_c =3D sk;
>>
>> =A0 =A0 =A0 l2cap_pi(sk)->next_c =3D l->head;
>> =A0 =A0 =A0 l2cap_pi(sk)->prev_c =3D NULL;
>> =A0 =A0 =A0 l->head =3D sk;
>> + =A0 =A0 write_unlock_bh(&l->lock);
>> =A0}
>
> a general rule of thumb is that function starting with __ are the ones
> that don't use any kind of locking.

Ok. I noted this after send the patch. :-(

>
> Regards
>
> Marcel
>
>
>



--=20
Gustavo F. Padovan

Computer Engineering Student
Institute of Computing - IC
University of Campinas - UNICAMP

email: gustavo@las.ic.unicamp.br
gtalk: gfpadovan@gmail.com
mobile: +55 19 81030803