2011-03-23 14:46:37

by Andre Dieb Martins

[permalink] [raw]
Subject: [PATCH 1/5] Inline ATT dump functions

---
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



2011-03-24 20:33:06

by Gustavo Padovan

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

Hi Andr?,

Please stop the top-posting on this mailing list.

* Andr? Dieb <[email protected]> [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

2011-03-24 12:59:28

by Andre Dieb Martins

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

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 <[email protected]> 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 <[email protected]> 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
>

2011-03-24 11:11:07

by Szymon Janc

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

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 <[email protected]> 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

2011-03-24 10:22:20

by Andre Dieb Martins

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

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 <[email protected]> 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
>

2011-03-24 09:24:52

by Johan Hedberg

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

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

2011-03-23 14:46:39

by Andre Dieb Martins

[permalink] [raw]
Subject: [PATCH 3/5] Add parsing for ATT Write Command

---
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


2011-03-23 14:46:40

by Andre Dieb Martins

[permalink] [raw]
Subject: [PATCH 4/5] Add parsing for ATT Signed Write

---
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


2011-03-23 14:46:41

by Andre Dieb Martins

[permalink] [raw]
Subject: [PATCH 5/5] Fix handle formatting for ATT Handle Notify

---
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


2011-03-23 14:46:38

by Andre Dieb Martins

[permalink] [raw]
Subject: [PATCH 2/5] Add parsing for ATT Write Request

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