[PATCH 1/2] Bluetooth: Move scope of state_to_string

[PATCH 1/2] Bluetooth: Move scope of state_to_string

[Emeltchenko Andrei]

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

Function state_to_string will be used in other files.

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

diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 4a82ca0..0a47c20 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -129,6 +129,33 @@ enum {
 	BT_CLOSED
 };
 
+/* If unused will be removed by compiler */
+static inline char *state_to_string(int state)
+{
+	switch (state) {
+	case BT_CONNECTED:
+		return "BT_CONNECTED";
+	case BT_OPEN:
+		return "BT_OPEN";
+	case BT_BOUND:
+		return "BT_BOUND";
+	case BT_LISTEN:
+		return "BT_LISTEN";
+	case BT_CONNECT:
+		return "BT_CONNECT";
+	case BT_CONNECT2:
+		return "BT_CONNECT2";
+	case BT_CONFIG:
+		return "BT_CONFIG";
+	case BT_DISCONN:
+		return "BT_DISCONN";
+	case BT_CLOSED:
+		return "BT_CLOSED";
+	}
+
+	return "invalid state";
+}
+
 /* BD Address */
 typedef struct {
 	__u8 b[6];
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 8efac78..252a96e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -215,32 +215,6 @@ static u16 l2cap_alloc_cid(struct l2cap_conn *conn)
 	return 0;
 }
 
-static char *state_to_string(int state)
-{
-	switch(state) {
-	case BT_CONNECTED:
-		return "BT_CONNECTED";
-	case BT_OPEN:
-		return "BT_OPEN";
-	case BT_BOUND:
-		return "BT_BOUND";
-	case BT_LISTEN:
-		return "BT_LISTEN";
-	case BT_CONNECT:
-		return "BT_CONNECT";
-	case BT_CONNECT2:
-		return "BT_CONNECT2";
-	case BT_CONFIG:
-		return "BT_CONFIG";
-	case BT_DISCONN:
-		return "BT_DISCONN";
-	case BT_CLOSED:
-		return "BT_CLOSED";
-	}
-
-	return "invalid state";
-}
-
 static void l2cap_state_change(struct l2cap_chan *chan, int state)
 {
 	BT_DBG("%p %s -> %s", chan, state_to_string(chan->state),
-- 
1.7.9

[PATCH 2/2] Bluetooth: Use symbolic name for state in debug

[Emeltchenko Andrei]

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

Use state_to_string function in debug

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/l2cap.h |    3 ++-
 net/bluetooth/hci_conn.c      |    2 +-
 net/bluetooth/l2cap_core.c    |    5 +++--
 net/bluetooth/l2cap_sock.c    |    2 +-
 4 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 42fdbb8..bbb0e21 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -612,7 +612,8 @@ static inline void l2cap_chan_put(struct l2cap_chan *c)
 static inline void l2cap_set_timer(struct l2cap_chan *chan,
 					struct delayed_work *work, long timeout)
 {
-	BT_DBG("chan %p state %d timeout %ld", chan, chan->state, timeout);
+	BT_DBG("chan %p state %s timeout %ld", chan,
+					state_to_string(chan->state), timeout);
 
 	if (!cancel_delayed_work(work))
 		l2cap_chan_hold(chan);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 6d15cba..f7a090c 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -282,7 +282,7 @@ static void hci_conn_timeout(struct work_struct *work)
 							disc_work.work);
 	__u8 reason;
 
-	BT_DBG("conn %p state %d", conn, conn->state);
+	BT_DBG("conn %p state %s", conn, state_to_string(conn->state));
 
 	if (atomic_read(&conn->refcnt))
 		return;
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 252a96e..88968e7 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -231,7 +231,7 @@ static void l2cap_chan_timeout(struct work_struct *work)
 	struct sock *sk = chan->sk;
 	int reason;
 
-	BT_DBG("chan %p state %d", chan, chan->state);
+	BT_DBG("chan %p state %s", chan, state_to_string(chan->state));
 
 	lock_sock(sk);
 
@@ -413,7 +413,8 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
 	struct l2cap_conn *conn = chan->conn;
 	struct sock *sk = chan->sk;
 
-	BT_DBG("chan %p state %d socket %p", chan, chan->state, sk->sk_socket);
+	BT_DBG("chan %p state %s sk %p", chan,
+					state_to_string(chan->state), sk);
 
 	switch (chan->state) {
 	case BT_LISTEN:
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 138fe34..b48d6c1 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -783,7 +783,7 @@ static void l2cap_sock_kill(struct sock *sk)
 	if (!sock_flag(sk, SOCK_ZAPPED) || sk->sk_socket)
 		return;
 
-	BT_DBG("sk %p state %d", sk, sk->sk_state);
+	BT_DBG("sk %p state %s", sk, state_to_string(sk->sk_state));
 
 	/* Kill poor orphan */
 
-- 
1.7.9

Re: [PATCH 1/2] Bluetooth: Move scope of state_to_string

[Marcel Holtmann]

Hi Andrei,

> Function state_to_string will be used in other files.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  include/net/bluetooth/bluetooth.h |   27 +++++++++++++++++++++++++++
>  net/bluetooth/l2cap_core.c        |   26 --------------------------
>  2 files changed, 27 insertions(+), 26 deletions(-)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 4a82ca0..0a47c20 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -129,6 +129,33 @@ enum {
>  	BT_CLOSED
>  };
>  
> +/* If unused will be removed by compiler */
> +static inline char *state_to_string(int state)
> +{
> +	switch (state) {
> +	case BT_CONNECTED:
> +		return "BT_CONNECTED";
> +	case BT_OPEN:
> +		return "BT_OPEN";
> +	case BT_BOUND:
> +		return "BT_BOUND";
> +	case BT_LISTEN:
> +		return "BT_LISTEN";
> +	case BT_CONNECT:
> +		return "BT_CONNECT";
> +	case BT_CONNECT2:
> +		return "BT_CONNECT2";
> +	case BT_CONFIG:
> +		return "BT_CONFIG";
> +	case BT_DISCONN:
> +		return "BT_DISCONN";
> +	case BT_CLOSED:
> +		return "BT_CLOSED";
> +	}
> +
> +	return "invalid state";
> +}
> +

so long term, we need to keep socket states inside l2cap_sock.c and
L2CAP states inside l2cap_core.c. Since L2CAP core engine should use the
states from the specification and not try to fit into socket states.

I see a little bit of value here in having them in the debug log, but
making them public like this seems a big overhead. Did you measure the
size difference. And most likely some state lookup table is better.

Regards

Marcel

Re: [PATCH 1/2] Bluetooth: Move scope of state_to_string

[Emeltchenko Andrei]

Hi Marcel,

On Wed, Feb 15, 2012 at 01:20:14PM +0100, Marcel Holtmann wrote:
> Hi Andrei,
> 
> > Function state_to_string will be used in other files.
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  include/net/bluetooth/bluetooth.h |   27 +++++++++++++++++++++++++++
> >  net/bluetooth/l2cap_core.c        |   26 --------------------------
> >  2 files changed, 27 insertions(+), 26 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index 4a82ca0..0a47c20 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -129,6 +129,33 @@ enum {
> >  	BT_CLOSED
> >  };
> >  
> > +/* If unused will be removed by compiler */
> > +static inline char *state_to_string(int state)
> > +{
> > +	switch (state) {
> > +	case BT_CONNECTED:
> > +		return "BT_CONNECTED";
> > +	case BT_OPEN:
> > +		return "BT_OPEN";
> > +	case BT_BOUND:
> > +		return "BT_BOUND";
> > +	case BT_LISTEN:
> > +		return "BT_LISTEN";
> > +	case BT_CONNECT:
> > +		return "BT_CONNECT";
> > +	case BT_CONNECT2:
> > +		return "BT_CONNECT2";
> > +	case BT_CONFIG:
> > +		return "BT_CONFIG";
> > +	case BT_DISCONN:
> > +		return "BT_DISCONN";
> > +	case BT_CLOSED:
> > +		return "BT_CLOSED";
> > +	}
> > +
> > +	return "invalid state";
> > +}
> > +
> 
> so long term, we need to keep socket states inside l2cap_sock.c and
> L2CAP states inside l2cap_core.c. Since L2CAP core engine should use the
> states from the specification and not try to fit into socket states.
> 
> I see a little bit of value here in having them in the debug log, but
> making them public like this seems a big overhead. Did you measure the
> size difference. And most likely some state lookup table is better.

If unused this shall be removed by compiler or what do you mean here?

Do you think l2cap.h is better place?

Best regards 
Andrei Emeltchenko 

Re: [PATCH 1/2] Bluetooth: Move scope of state_to_string

[Emeltchenko Andrei]

Hi Marcel,

On Wed, Feb 15, 2012 at 01:20:14PM +0100, Marcel Holtmann wrote:
> Hi Andrei,
> 
> > Function state_to_string will be used in other files.
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  include/net/bluetooth/bluetooth.h |   27 +++++++++++++++++++++++++++
> >  net/bluetooth/l2cap_core.c        |   26 --------------------------
> >  2 files changed, 27 insertions(+), 26 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index 4a82ca0..0a47c20 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -129,6 +129,33 @@ enum {
> >  	BT_CLOSED
> >  };
> >  
> > +/* If unused will be removed by compiler */
> > +static inline char *state_to_string(int state)
> > +{
> > +	switch (state) {
> > +	case BT_CONNECTED:
> > +		return "BT_CONNECTED";
> > +	case BT_OPEN:
> > +		return "BT_OPEN";
> > +	case BT_BOUND:
> > +		return "BT_BOUND";
> > +	case BT_LISTEN:
> > +		return "BT_LISTEN";
> > +	case BT_CONNECT:
> > +		return "BT_CONNECT";
> > +	case BT_CONNECT2:
> > +		return "BT_CONNECT2";
> > +	case BT_CONFIG:
> > +		return "BT_CONFIG";
> > +	case BT_DISCONN:
> > +		return "BT_DISCONN";
> > +	case BT_CLOSED:
> > +		return "BT_CLOSED";
> > +	}
> > +
> > +	return "invalid state";
> > +}
> > +
> 
> so long term, we need to keep socket states inside l2cap_sock.c and
> L2CAP states inside l2cap_core.c. Since L2CAP core engine should use the
> states from the specification and not try to fit into socket states.
> 
> I see a little bit of value here in having them in the debug log, but
> making them public like this seems a big overhead. Did you measure the
> size difference. 

size show exactly the same size after applying this patch with my config.
(dynamic debug enabled)

size net/bluetooth/bluetooth.ko 
   text	   data	    bss	    dec	    hex	filename
 196404	  12912	    644	 209960	  33428	net/bluetooth/bluetooth.ko


> And most likely some state lookup table is better.

This can be done, BTW: unused functions will be removed by compiler, what
about lookup table? I assume the same.

Best regards 
Andrei Emeltchenko 

Re: [PATCH 1/2] Bluetooth: Move scope of state_to_string

[Marcel Holtmann]

Hi Andrei,

> > > Function state_to_string will be used in other files.
> > > 
> > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > ---
> > >  include/net/bluetooth/bluetooth.h |   27 +++++++++++++++++++++++++++
> > >  net/bluetooth/l2cap_core.c        |   26 --------------------------
> > >  2 files changed, 27 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > > index 4a82ca0..0a47c20 100644
> > > --- a/include/net/bluetooth/bluetooth.h
> > > +++ b/include/net/bluetooth/bluetooth.h
> > > @@ -129,6 +129,33 @@ enum {
> > >  	BT_CLOSED
> > >  };
> > >  
> > > +/* If unused will be removed by compiler */
> > > +static inline char *state_to_string(int state)
> > > +{
> > > +	switch (state) {
> > > +	case BT_CONNECTED:
> > > +		return "BT_CONNECTED";
> > > +	case BT_OPEN:
> > > +		return "BT_OPEN";
> > > +	case BT_BOUND:
> > > +		return "BT_BOUND";
> > > +	case BT_LISTEN:
> > > +		return "BT_LISTEN";
> > > +	case BT_CONNECT:
> > > +		return "BT_CONNECT";
> > > +	case BT_CONNECT2:
> > > +		return "BT_CONNECT2";
> > > +	case BT_CONFIG:
> > > +		return "BT_CONFIG";
> > > +	case BT_DISCONN:
> > > +		return "BT_DISCONN";
> > > +	case BT_CLOSED:
> > > +		return "BT_CLOSED";
> > > +	}
> > > +
> > > +	return "invalid state";
> > > +}
> > > +
> > 
> > so long term, we need to keep socket states inside l2cap_sock.c and
> > L2CAP states inside l2cap_core.c. Since L2CAP core engine should use the
> > states from the specification and not try to fit into socket states.
> > 
> > I see a little bit of value here in having them in the debug log, but
> > making them public like this seems a big overhead. Did you measure the
> > size difference. 
> 
> size show exactly the same size after applying this patch with my config.
> (dynamic debug enabled)
> 
> size net/bluetooth/bluetooth.ko 
>    text	   data	    bss	    dec	    hex	filename
>  196404	  12912	    644	 209960	  33428	net/bluetooth/bluetooth.ko

then I am fine with this. Just send an updated patch against the latest
tree.

> > And most likely some state lookup table is better.
> 
> This can be done, BTW: unused functions will be removed by compiler, what
> about lookup table? I assume the same.

Depends on where it is. If it needs to be a public symbol because it is
used from multiple modules, then most likely not.

Regards

Marcel

Re: [PATCH 1/2] Bluetooth: Move scope of state_to_string

[Emeltchenko Andrei]

Hi Marcel,

On Fri, Feb 17, 2012 at 10:12:10AM +0100, Marcel Holtmann wrote:
> Hi Andrei,
> 
> > > > Function state_to_string will be used in other files.
> > > > 
> > > so long term, we need to keep socket states inside l2cap_sock.c and
> > > L2CAP states inside l2cap_core.c. Since L2CAP core engine should use the
> > > states from the specification and not try to fit into socket states.
> > > 
> > > I see a little bit of value here in having them in the debug log, but
> > > making them public like this seems a big overhead. Did you measure the
> > > size difference. 
> > 
> > size show exactly the same size after applying this patch with my config.
> > (dynamic debug enabled)
> > 
> > size net/bluetooth/bluetooth.ko 
> >    text	   data	    bss	    dec	    hex	filename
> >  196404	  12912	    644	 209960	  33428	net/bluetooth/bluetooth.ko
> 
> then I am fine with this. Just send an updated patch against the latest
> tree.

If we don't want it to be in bluetooth.h I can export only function
declaration or put it in l2cap.h. What is better?

Best regards 
Andrei Emeltchenko 

Re: [PATCH 1/2] Bluetooth: Move scope of state_to_string

[Marcel Holtmann]

Hi Andrei,

> > > > > Function state_to_string will be used in other files.
> > > > > 
> > > > so long term, we need to keep socket states inside l2cap_sock.c and
> > > > L2CAP states inside l2cap_core.c. Since L2CAP core engine should use the
> > > > states from the specification and not try to fit into socket states.
> > > > 
> > > > I see a little bit of value here in having them in the debug log, but
> > > > making them public like this seems a big overhead. Did you measure the
> > > > size difference. 
> > > 
> > > size show exactly the same size after applying this patch with my config.
> > > (dynamic debug enabled)
> > > 
> > > size net/bluetooth/bluetooth.ko 
> > >    text	   data	    bss	    dec	    hex	filename
> > >  196404	  12912	    644	 209960	  33428	net/bluetooth/bluetooth.ko
> > 
> > then I am fine with this. Just send an updated patch against the latest
> > tree.
> 
> If we don't want it to be in bluetooth.h I can export only function
> declaration or put it in l2cap.h. What is better?

if the size is the same, then put it in bluetooth.h.

Regards

Marcel