[PATCH 1/5] Inline ATT dump functions

[PATCH 1/5] Inline ATT dump functions

[Andre Dieb Martins]

---
 parser/att.c |   32 ++++++++++++++++----------------
 1 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/parser/att.c b/parser/att.c
index 7b8b83c..6c1d0d8 100644
--- a/parser/att.c
+++ b/parser/att.c
@@ -257,7 +257,7 @@ static const char *uuid2str(uint16_t uuid)
 	}
 }
 
-static void att_error_dump(int level, struct frame *frm)
+static inline void att_error_dump(int level, struct frame *frm)
 {
 	uint8_t op = get_u8(frm);
 	uint16_t handle = btohs(htons(get_u16(frm)));
@@ -270,7 +270,7 @@ static void att_error_dump(int level, struct frame *frm)
 	printf("%s (0x%.2x) on handle 0x%2.2x\n", attop2str(op), op, handle);
 }
 
-static void att_mtu_req_dump(int level, struct frame *frm)
+static inline void att_mtu_req_dump(int level, struct frame *frm)
 {
 	uint16_t client_rx_mtu = btohs(htons(get_u16(frm)));
 
@@ -278,7 +278,7 @@ static void att_mtu_req_dump(int level, struct frame *frm)
 	printf("client rx mtu %d\n", client_rx_mtu);
 }
 
-static void att_mtu_resp_dump(int level, struct frame *frm)
+static inline void att_mtu_resp_dump(int level, struct frame *frm)
 {
 	uint16_t server_rx_mtu = btohs(htons(get_u16(frm)));
 
@@ -286,7 +286,7 @@ static void att_mtu_resp_dump(int level, struct frame *frm)
 	printf("server rx mtu %d\n", server_rx_mtu);
 }
 
-static void att_find_info_req_dump(int level, struct frame *frm)
+static inline void att_find_info_req_dump(int level, struct frame *frm)
 {
 	uint16_t start = btohs(htons(get_u16(frm)));
 	uint16_t end = btohs(htons(get_u16(frm)));
@@ -295,7 +295,7 @@ static void att_find_info_req_dump(int level, struct frame *frm)
 	printf("start 0x%2.2x, end 0x%2.2x\n", start, end);
 }
 
-static void att_find_info_resp_dump(int level, struct frame *frm)
+static inline void att_find_info_resp_dump(int level, struct frame *frm)
 {
 	uint8_t fmt = get_u8(frm);
 
@@ -330,7 +330,7 @@ static void att_find_info_resp_dump(int level, struct frame *frm)
 	}
 }
 
-static void att_find_by_type_req_dump(int level, struct frame *frm)
+static inline void att_find_by_type_req_dump(int level, struct frame *frm)
 {
 	uint16_t start = btohs(htons(get_u16(frm)));
 	uint16_t end = btohs(htons(get_u16(frm)));
@@ -346,7 +346,7 @@ static void att_find_by_type_req_dump(int level, struct frame *frm)
 	printf("\n");
 }
 
-static void att_find_by_type_resp_dump(int level, struct frame *frm)
+static inline void att_find_by_type_resp_dump(int level, struct frame *frm)
 {
 	while (frm->len > 0) {
 		uint16_t uuid = btohs(htons(get_u16(frm)));
@@ -358,7 +358,7 @@ static void att_find_by_type_resp_dump(int level, struct frame *frm)
 	}
 }
 
-static void att_read_by_type_req_dump(int level, struct frame *frm)
+static inline void att_read_by_type_req_dump(int level, struct frame *frm)
 {
 	uint16_t start = btohs(htons(get_u16(frm)));
 	uint16_t end = btohs(htons(get_u16(frm)));
@@ -385,7 +385,7 @@ static void att_read_by_type_req_dump(int level, struct frame *frm)
 	}
 }
 
-static void att_read_by_type_resp_dump(int level, struct frame *frm)
+static inline void att_read_by_type_resp_dump(int level, struct frame *frm)
 {
 	uint8_t length = get_u8(frm);
 
@@ -406,7 +406,7 @@ static void att_read_by_type_resp_dump(int level, struct frame *frm)
 	}
 }
 
-static void att_read_req_dump(int level, struct frame *frm)
+static inline void att_read_req_dump(int level, struct frame *frm)
 {
 	uint16_t handle = btohs(htons(get_u16(frm)));
 
@@ -414,7 +414,7 @@ static void att_read_req_dump(int level, struct frame *frm)
 	printf("handle 0x%2.2x\n", handle);
 }
 
-static void att_read_blob_req_dump(int level, struct frame *frm)
+static inline void att_read_blob_req_dump(int level, struct frame *frm)
 {
 	uint16_t handle = btohs(htons(get_u16(frm)));
 	uint16_t offset = btohs(htons(get_u16(frm)));
@@ -423,7 +423,7 @@ static void att_read_blob_req_dump(int level, struct frame *frm)
 	printf("handle 0x%4.4x offset 0x%4.4x\n", handle, offset);
 }
 
-static void att_read_blob_resp_dump(int level, struct frame *frm)
+static inline void att_read_blob_resp_dump(int level, struct frame *frm)
 {
 	p_indent(level, frm);
 	printf("value");
@@ -433,7 +433,7 @@ static void att_read_blob_resp_dump(int level, struct frame *frm)
 	printf("\n");
 }
 
-static void att_read_multi_req_dump(int level, struct frame *frm)
+static inline void att_read_multi_req_dump(int level, struct frame *frm)
 {
 	p_indent(level, frm);
 	printf("Handles\n");
@@ -444,7 +444,7 @@ static void att_read_multi_req_dump(int level, struct frame *frm)
 	}
 }
 
-static void att_read_multi_resp_dump(int level, struct frame *frm)
+static inline void att_read_multi_resp_dump(int level, struct frame *frm)
 {
 	p_indent(level, frm);
 	printf("values");
@@ -454,7 +454,7 @@ static void att_read_multi_resp_dump(int level, struct frame *frm)
 	printf("\n");
 }
 
-static void att_read_by_group_resp_dump(int level, struct frame *frm)
+static inline void att_read_by_group_resp_dump(int level, struct frame *frm)
 {
 	uint8_t length = get_u8(frm);
 
@@ -476,7 +476,7 @@ static void att_read_by_group_resp_dump(int level, struct frame *frm)
 	}
 }
 
-static void att_handle_notify_dump(int level, struct frame *frm)
+static inline void att_handle_notify_dump(int level, struct frame *frm)
 {
 	uint16_t handle = btohs(htons(get_u16(frm)));
 
-- 
1.7.1

[PATCH 2/5] Add parsing for ATT Write Request

[Andre Dieb Martins]

Note we do not need extra parsing for ATT Write Response as it only has one
field (opcode).
---
 parser/att.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/parser/att.c b/parser/att.c
index 6c1d0d8..6a1b1b5 100644
--- a/parser/att.c
+++ b/parser/att.c
@@ -476,6 +476,18 @@ static inline void att_read_by_group_resp_dump(int level, struct frame *frm)
 	}
 }
 
+static inline void att_write_req_dump(int level, struct frame *frm)
+{
+	uint16_t handle = btohs(htons(get_u16(frm)));
+
+	p_indent(level, frm);
+	printf("handle 0x%4.4x value ", handle);
+
+	while (frm->len > 0)
+		printf(" 0x%2.2x", get_u8(frm));
+	printf("\n");
+}
+
 static inline void att_handle_notify_dump(int level, struct frame *frm)
 {
 	uint16_t handle = btohs(htons(get_u16(frm)));
@@ -549,6 +561,9 @@ void att_dump(int level, struct frame *frm)
 		case ATT_OP_READ_BY_GROUP_RESP:
 			att_read_by_group_resp_dump(level + 1, frm);
 			break;
+		case ATT_OP_WRITE_REQ:
+			att_write_req_dump(level + 1, frm);
+			break;
 		case ATT_OP_HANDLE_NOTIFY:
 			att_handle_notify_dump(level + 1, frm);
 			break;
-- 
1.7.1

[PATCH 3/5] Add parsing for ATT Write Command

[Andre Dieb Martins]

---
 parser/att.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/parser/att.c b/parser/att.c
index 6a1b1b5..b85ff0c 100644
--- a/parser/att.c
+++ b/parser/att.c
@@ -562,6 +562,7 @@ void att_dump(int level, struct frame *frm)
 			att_read_by_group_resp_dump(level + 1, frm);
 			break;
 		case ATT_OP_WRITE_REQ:
+		case ATT_OP_WRITE_CMD:
 			att_write_req_dump(level + 1, frm);
 			break;
 		case ATT_OP_HANDLE_NOTIFY:
-- 
1.7.1

[PATCH 4/5] Add parsing for ATT Signed Write

[Andre Dieb Martins]

---
 parser/att.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/parser/att.c b/parser/att.c
index b85ff0c..4141f99 100644
--- a/parser/att.c
+++ b/parser/att.c
@@ -488,6 +488,25 @@ static inline void att_write_req_dump(int level, struct frame *frm)
 	printf("\n");
 }
 
+static inline void att_signed_write_dump(int level, struct frame *frm)
+{
+	uint16_t handle = btohs(htons(get_u16(frm)));
+	int value_len = frm->len - 12; /* handle:2 already accounted, sig: 12 */
+
+	p_indent(level, frm);
+	printf("handle 0x%4.4x value ", handle);
+
+	while (value_len--)
+		printf(" 0x%2.2x", get_u8(frm));
+	printf("\n");
+
+	p_indent(level, frm);
+	printf("auth signature ");
+	while (frm->len > 0)
+		printf(" 0x%2.2x", get_u8(frm));
+	printf("\n");
+}
+
 static inline void att_handle_notify_dump(int level, struct frame *frm)
 {
 	uint16_t handle = btohs(htons(get_u16(frm)));
@@ -565,6 +584,9 @@ void att_dump(int level, struct frame *frm)
 		case ATT_OP_WRITE_CMD:
 			att_write_req_dump(level + 1, frm);
 			break;
+		case ATT_OP_SIGNED_WRITE_CMD:
+			att_signed_write_dump(level + 1, frm);
+			break;
 		case ATT_OP_HANDLE_NOTIFY:
 			att_handle_notify_dump(level + 1, frm);
 			break;
-- 
1.7.1

[PATCH 5/5] Fix handle formatting for ATT Handle Notify

[Andre Dieb Martins]

---
 parser/att.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/parser/att.c b/parser/att.c
index 4141f99..5e589c4 100644
--- a/parser/att.c
+++ b/parser/att.c
@@ -512,7 +512,7 @@ static inline void att_handle_notify_dump(int level, struct frame *frm)
 	uint16_t handle = btohs(htons(get_u16(frm)));
 
 	p_indent(level, frm);
-	printf("handle 0x%2.2x\n", handle);
+	printf("handle 0x%4.4x\n", handle);
 
 	p_indent(level, frm);
 	printf("value ");
-- 
1.7.1

Re: [PATCH 1/5] Inline ATT dump functions

[Johan Hedberg]

Hi André,

On Wed, Mar 23, 2011, Andre Dieb Martins wrote:
> ---
>  parser/att.c |   32 ++++++++++++++++----------------
>  1 files changed, 16 insertions(+), 16 deletions(-)

Do you have some measurements that show that inlining actually has a
noticable effect? You're also missing the justification for this change
in the commit message.

Johan

Re: [PATCH 1/5] Inline ATT dump functions

[André Dieb]

No, I don't. This patch is merely because all hcidump parsers (hci,
l2cap, etc) are done inline.

On Thu, Mar 24, 2011 at 6:24 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi André,
>
> On Wed, Mar 23, 2011, Andre Dieb Martins wrote:
>> ---
>>  parser/att.c |   32 ++++++++++++++++----------------
>>  1 files changed, 16 insertions(+), 16 deletions(-)
>
> Do you have some measurements that show that inlining actually has a
> noticable effect? You're also missing the justification for this change
> in the commit message.
>
> Johan
>

Re: [PATCH 1/5] Inline ATT dump functions

[Szymon Janc]

Hi,

> No, I don't. This patch is merely because all hcidump parsers (hci,
> l2cap, etc) are done inline.
> 
> On Thu, Mar 24, 2011 at 6:24 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> > Hi André,
> >
> > On Wed, Mar 23, 2011, Andre Dieb Martins wrote:
> >> ---
> >>  parser/att.c |   32 ++++++++++++++++----------------
> >>  1 files changed, 16 insertions(+), 16 deletions(-)
> >
> > Do you have some measurements that show that inlining actually has a
> > noticable effect? You're also missing the justification for this change
> > in the commit message.

Personally I think that we should have most (if not all) of static functions
uninlined and leave inlining decision to compiler (i.e. -finline-functions)

Inlining functions based on "strong feelings and experience" or "function
is short" without proper profiling this is just pure guesswork and can easily
lead to unwanted results.

-- 
BR
Szymon Janc

Re: [PATCH 1/5] Inline ATT dump functions

[André Dieb]

Hello,

In case most agree with this, I can change my patch and provide some
patches changing old code.

Cheers

On Thu, Mar 24, 2011 at 8:11 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> Hi,
>
>> No, I don't. This patch is merely because all hcidump parsers (hci,
>> l2cap, etc) are done inline.
>>
>> On Thu, Mar 24, 2011 at 6:24 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> > Hi André,
>> >
>> > On Wed, Mar 23, 2011, Andre Dieb Martins wrote:
>> >> ---
>> >>  parser/att.c |   32 ++++++++++++++++----------------
>> >>  1 files changed, 16 insertions(+), 16 deletions(-)
>> >
>> > Do you have some measurements that show that inlining actually has a
>> > noticable effect? You're also missing the justification for this change
>> > in the commit message.
>
> Personally I think that we should have most (if not all) of static functions
> uninlined and leave inlining decision to compiler (i.e. -finline-functions)
>
> Inlining functions based on "strong feelings and experience" or "function
> is short" without proper profiling this is just pure guesswork and can easily
> lead to unwanted results.
>
> --
> BR
> Szymon Janc
>

Re: [PATCH 1/5] Inline ATT dump functions

[Gustavo F. Padovan]

Hi André,

Please stop the top-posting on this mailing list.

* André Dieb <andre.dieb@signove.com> [2011-03-24 09:59:28 -0300]:

> Hello,
> 
> In case most agree with this, I can change my patch and provide some
> patches changing old code.

I agree, let the compiler decide. It know exactly how to handle this.

-- 
Gustavo F. Padovan
http://profusion.mobi