2010-07-09 10:49:29

by Suraj Sumangala

[permalink] [raw]
Subject: [PATCH v4 1/3] Implements hci_reassembly to reassemble Rx packets

Implements feature to reassemble HCI frames received from an input stream.

Signed-off-by: Suraj Sumangala <[email protected]>
---
net/bluetooth/hci_core.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 115 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 5e83f8e..3eb4c1b 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1030,6 +1030,121 @@ int hci_recv_frame(struct sk_buff *skb)
}
EXPORT_SYMBOL(hci_recv_frame);

+static int hci_reassembly(struct hci_dev *hdev, int type, void *data,
+ int count, struct sk_buff **skb_ptr, int *remain)
+{
+ int len = 0;
+ int header_len = 0;
+ struct sk_buff *skb = *skb_ptr;
+ struct { struct bt_skb_cb cb_info; int expect; } *scb;
+
+ *remain = count;
+
+ if (type < HCI_ACLDATA_PKT || type > HCI_EVENT_PKT)
+ return -EILSEQ;
+
+ if (!skb) {
+
+ switch (type) {
+ case HCI_ACLDATA_PKT:
+ len = HCI_MAX_FRAME_SIZE;
+ header_len = HCI_ACL_HDR_SIZE;
+ break;
+ case HCI_EVENT_PKT:
+ len = HCI_MAX_EVENT_SIZE;
+ header_len = HCI_EVENT_HDR_SIZE;
+ break;
+ case HCI_SCODATA_PKT:
+ len = HCI_MAX_SCO_SIZE;
+ header_len = HCI_SCO_HDR_SIZE;
+ break;
+ }
+
+ skb = bt_skb_alloc(len, GFP_ATOMIC);
+
+ if (!skb)
+ return -ENOMEM;
+
+ scb = (void *) skb->cb;
+ scb->expect = header_len;
+ scb->cb_info.pkt_type = (__u8)type;
+
+ skb->dev = (void *) hdev;
+ *skb_ptr = skb;
+
+ }
+
+ while (count) {
+
+ scb = (void *) skb->cb;
+ len = min(scb->expect, count);
+
+ memcpy(skb_put(skb, len), data, len);
+
+ count -= len;
+ data += len;
+ scb->expect -= len;
+ *remain = count;
+
+ switch (type) {
+ case HCI_EVENT_PKT:
+ if (skb->len == HCI_EVENT_HDR_SIZE) {
+ struct hci_event_hdr *h = hci_event_hdr(skb);
+ scb->expect = h->plen;
+
+ if (skb_tailroom(skb) < scb->expect) {
+ kfree_skb(skb);
+ *skb_ptr = NULL;
+
+ return -ENOMEM;
+ }
+ }
+ break;
+
+ case HCI_ACLDATA_PKT:
+ if (skb->len == HCI_ACL_HDR_SIZE) {
+ struct hci_acl_hdr *h = hci_acl_hdr(skb);
+ scb->expect = __le16_to_cpu(h->dlen);
+
+ if (skb_tailroom(skb) < scb->expect) {
+ kfree_skb(skb);
+ *skb_ptr = NULL;
+
+ return -ENOMEM;
+ }
+ }
+ break;
+
+ case HCI_SCODATA_PKT:
+ if (skb->len == HCI_SCO_HDR_SIZE) {
+ struct hci_sco_hdr *h = hci_sco_hdr(skb);
+ scb->expect = h->dlen;
+
+ if (skb_tailroom(skb) < scb->expect) {
+ kfree_skb(skb);
+ *skb_ptr = NULL;
+
+ return -ENOMEM;
+ }
+ }
+ break;
+ }
+
+ if (scb->expect == 0) {
+
+ /* Complete frame */
+ bt_cb(skb)->pkt_type = type;
+ hci_recv_frame(skb);
+
+ *skb_ptr = NULL;
+
+ return 0;
+ }
+
+ }
+
+ return 0;
+}
/* Receive packet type fragment */
#define __reassembly(hdev, type) ((hdev)->reassembly[(type) - 2])

--
1.7.0.4





2010-07-09 13:16:06

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH v4 1/3] Implements hci_reassembly to reassemble Rx packets

Hi Suraj,

you do need to fix your email or at least add a From: line. Otherwise
when I apply the patch this gets screwed up.

> Implements feature to reassemble HCI frames received from an input stream.
>
> Signed-off-by: Suraj Sumangala <[email protected]>
> ---
> net/bluetooth/hci_core.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 115 insertions(+), 0 deletions(-)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 5e83f8e..3eb4c1b 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1030,6 +1030,121 @@ int hci_recv_frame(struct sk_buff *skb)
> }
> EXPORT_SYMBOL(hci_recv_frame);
>
> +static int hci_reassembly(struct hci_dev *hdev, int type, void *data,
> + int count, struct sk_buff **skb_ptr, int *remain)

Why are we not not just return remain and err combined. Negative's are
errors positives are remains.

> +{
> + int len = 0;
> + int header_len = 0;
> + struct sk_buff *skb = *skb_ptr;
> + struct { struct bt_skb_cb cb_info; int expect; } *scb;
> +
> + *remain = count;
> +
> + if (type < HCI_ACLDATA_PKT || type > HCI_EVENT_PKT)
> + return -EILSEQ;
> +
> + if (!skb) {
> +
> + switch (type) {

Remove this empty line here.

> + case HCI_ACLDATA_PKT:
> + len = HCI_MAX_FRAME_SIZE;
> + header_len = HCI_ACL_HDR_SIZE;
> + break;

The break needs to be on the same line as the code.

> + case HCI_EVENT_PKT:
> + len = HCI_MAX_EVENT_SIZE;
> + header_len = HCI_EVENT_HDR_SIZE;
> + break;
> + case HCI_SCODATA_PKT:
> + len = HCI_MAX_SCO_SIZE;
> + header_len = HCI_SCO_HDR_SIZE;
> + break;
> + }
> +
> + skb = bt_skb_alloc(len, GFP_ATOMIC);
> +
> + if (!skb)

Remove the empty line here.

Do we always need GFP_ATOMIC here. I would prefer that we make that an
option of the function caller.

> + return -ENOMEM;
> +
> + scb = (void *) skb->cb;
> + scb->expect = header_len;
> + scb->cb_info.pkt_type = (__u8)type;

It is "(__u8) type" when casting.

> +
> + skb->dev = (void *) hdev;
> + *skb_ptr = skb;
> +
> + }
> +
> + while (count) {
> +
> + scb = (void *) skb->cb;

Remove empty line here.

> + len = min(scb->expect, count);
> +
> + memcpy(skb_put(skb, len), data, len);
> +
> + count -= len;
> + data += len;
> + scb->expect -= len;
> + *remain = count;
> +
> + switch (type) {
> + case HCI_EVENT_PKT:
> + if (skb->len == HCI_EVENT_HDR_SIZE) {
> + struct hci_event_hdr *h = hci_event_hdr(skb);
> + scb->expect = h->plen;
> +
> + if (skb_tailroom(skb) < scb->expect) {
> + kfree_skb(skb);
> + *skb_ptr = NULL;
> +
> + return -ENOMEM;

Remove this empty line.

> + }
> + }
> + break;
> +
> + case HCI_ACLDATA_PKT:
> + if (skb->len == HCI_ACL_HDR_SIZE) {
> + struct hci_acl_hdr *h = hci_acl_hdr(skb);
> + scb->expect = __le16_to_cpu(h->dlen);
> +
> + if (skb_tailroom(skb) < scb->expect) {
> + kfree_skb(skb);
> + *skb_ptr = NULL;
> +
> + return -ENOMEM;

Same here.

> + }
> + }
> + break;
> +
> + case HCI_SCODATA_PKT:
> + if (skb->len == HCI_SCO_HDR_SIZE) {
> + struct hci_sco_hdr *h = hci_sco_hdr(skb);
> + scb->expect = h->dlen;
> +
> + if (skb_tailroom(skb) < scb->expect) {
> + kfree_skb(skb);
> + *skb_ptr = NULL;
> +
> + return -ENOMEM;

And here.

> + }
> + }
> + break;
> + }
> +
> + if (scb->expect == 0) {
> +
> + /* Complete frame */

Remove the empty line here.

> + bt_cb(skb)->pkt_type = type;
> + hci_recv_frame(skb);
> +
> + *skb_ptr = NULL;
> +
> + return 0;

And here.

> + }
> +
> + }

What is this empty line for. Please remove.

> +
> + return 0;
> +}
> /* Receive packet type fragment */

And here we need an extra empty line.

> #define __reassembly(hdev, type) ((hdev)->reassembly[(type) - 2])

Regards

Marcel



2010-07-09 12:59:12

by Gustavo Padovan

[permalink] [raw]
Subject: Re: [PATCH v4 3/3] Implemented HCI frame reassembly for Rx from stream

Hi Suraj,

* suraj <[email protected]> [2010-07-09 16:23:41 +0530]:

> Implemented frame reassembly implementation for reassembling fragments
> received from stream.
>
> Signed-off-by: Suraj Sumangala <[email protected]>
> ---
> include/net/bluetooth/hci_core.h | 2 +
> net/bluetooth/hci_core.c | 43
> ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 45 insertions(+), 0 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h
> b/include/net/bluetooth/hci_core.h
> index e42f6ed..cd89d66 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -119,6 +119,7 @@ struct hci_dev {
>
> struct sk_buff *sent_cmd;
> struct sk_buff *reassembly[3];
> + struct sk_buff *stream_reassembly;
>
> struct mutex req_lock;
> wait_queue_head_t req_wait_q;
> @@ -428,6 +429,7 @@ void hci_event_packet(struct hci_dev *hdev, struct
> sk_buff *skb);
>
> int hci_recv_frame(struct sk_buff *skb);
> int hci_recv_fragment(struct hci_dev *hdev, int type, void *data, int
> count);
> +int hci_recv_stream_fragment(struct hci_dev *hdev, void *data, int
> count);
>
> int hci_register_sysfs(struct hci_dev *hdev);
> void hci_unregister_sysfs(struct hci_dev *hdev);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index db6ca71..ee75c42 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -916,6 +916,8 @@ int hci_register_dev(struct hci_dev *hdev)
> for (i = 0; i < 3; i++)
> hdev->reassembly[i] = NULL;
>
> + hdev->stream_reassembly = NULL;
> +
> init_waitqueue_head(&hdev->req_wait_q);
> mutex_init(&hdev->req_lock);
>
> @@ -973,6 +975,8 @@ int hci_unregister_dev(struct hci_dev *hdev)
> for (i = 0; i < 3; i++)
> kfree_skb(hdev->reassembly[i]);
>
> + kfree_skb(hdev->stream_reassembly);
> +
> hci_notify(hdev, HCI_DEV_UNREG);
>
> if (hdev->rfkill) {
> @@ -1145,6 +1149,45 @@ static int hci_reassembly(struct hci_dev *hdev,
> int type, void *data,
>
> return 0;
> }
> +
> +#define __stream_reassembly(hdev) ((hdev)->stream_reassembly)

That's pointless. Your macro has about the same length of
hdev->stream_reassembly.

> +
> +int hci_recv_stream_fragment(struct hci_dev *hdev, void *data, int
> count)
> +{
> + int type;
> + int remaining = 0;
> + int err = 0;
> +
> + do {
> + struct sk_buff *skb = __stream_reassembly(hdev);
> + if (!skb) {
> + struct { char type; } *pkt;
> +
> + /* Start of the frame */
> + pkt = data;
> + type = pkt->type;
> +
> + data++;
> + count--;
> + } else
> + type = bt_cb(skb)->pkt_type;
> +
> + err = hci_reassembly(hdev, type, data, count, &skb, &remaining);
> +
> + if (err < 0)
> + return err;
> +
> + __stream_reassembly(hdev) = skb;
> +
> + data += (count - remaining);
> + count = remaining;
> +
> + } while (count);
> +
> + return err;
> +}
> +EXPORT_SYMBOL(hci_recv_stream_fragment);
> +
> /* Receive packet type fragment */
> #define __reassembly(hdev, type) ((hdev)->reassembly[(type) - 2])
>
> --
> 1.7.0.4
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Gustavo F. Padovan
http://padovan.org

2010-07-09 10:53:41

by Suraj Sumangala

[permalink] [raw]
Subject: [PATCH v4 3/3] Implemented HCI frame reassembly for Rx from stream

Implemented frame reassembly implementation for reassembling fragments
received from stream.

Signed-off-by: Suraj Sumangala <[email protected]>
---
include/net/bluetooth/hci_core.h | 2 +
net/bluetooth/hci_core.c | 43
++++++++++++++++++++++++++++++++++++++
2 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h
b/include/net/bluetooth/hci_core.h
index e42f6ed..cd89d66 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -119,6 +119,7 @@ struct hci_dev {

struct sk_buff *sent_cmd;
struct sk_buff *reassembly[3];
+ struct sk_buff *stream_reassembly;

struct mutex req_lock;
wait_queue_head_t req_wait_q;
@@ -428,6 +429,7 @@ void hci_event_packet(struct hci_dev *hdev, struct
sk_buff *skb);

int hci_recv_frame(struct sk_buff *skb);
int hci_recv_fragment(struct hci_dev *hdev, int type, void *data, int
count);
+int hci_recv_stream_fragment(struct hci_dev *hdev, void *data, int
count);

int hci_register_sysfs(struct hci_dev *hdev);
void hci_unregister_sysfs(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index db6ca71..ee75c42 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -916,6 +916,8 @@ int hci_register_dev(struct hci_dev *hdev)
for (i = 0; i < 3; i++)
hdev->reassembly[i] = NULL;

+ hdev->stream_reassembly = NULL;
+
init_waitqueue_head(&hdev->req_wait_q);
mutex_init(&hdev->req_lock);

@@ -973,6 +975,8 @@ int hci_unregister_dev(struct hci_dev *hdev)
for (i = 0; i < 3; i++)
kfree_skb(hdev->reassembly[i]);

+ kfree_skb(hdev->stream_reassembly);
+
hci_notify(hdev, HCI_DEV_UNREG);

if (hdev->rfkill) {
@@ -1145,6 +1149,45 @@ static int hci_reassembly(struct hci_dev *hdev,
int type, void *data,

return 0;
}
+
+#define __stream_reassembly(hdev) ((hdev)->stream_reassembly)
+
+int hci_recv_stream_fragment(struct hci_dev *hdev, void *data, int
count)
+{
+ int type;
+ int remaining = 0;
+ int err = 0;
+
+ do {
+ struct sk_buff *skb = __stream_reassembly(hdev);
+ if (!skb) {
+ struct { char type; } *pkt;
+
+ /* Start of the frame */
+ pkt = data;
+ type = pkt->type;
+
+ data++;
+ count--;
+ } else
+ type = bt_cb(skb)->pkt_type;
+
+ err = hci_reassembly(hdev, type, data, count, &skb, &remaining);
+
+ if (err < 0)
+ return err;
+
+ __stream_reassembly(hdev) = skb;
+
+ data += (count - remaining);
+ count = remaining;
+
+ } while (count);
+
+ return err;
+}
+EXPORT_SYMBOL(hci_recv_stream_fragment);
+
/* Receive packet type fragment */
#define __reassembly(hdev, type) ((hdev)->reassembly[(type) - 2])

--
1.7.0.4

2010-07-09 10:51:42

by Suraj Sumangala

[permalink] [raw]
Subject: [PATCH v4 2/3] Modified hci_recv_fragment() to use hci_reassembly

modified packet based reassembly function hci_recv_fragment() to use hci_reassembly()

Signed-off-by: Suraj Sumangala <[email protected]>
---
net/bluetooth/hci_core.c | 78 +++++++--------------------------------------
1 files changed, 12 insertions(+), 66 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 3eb4c1b..db6ca71 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1150,82 +1150,28 @@ static int hci_reassembly(struct hci_dev *hdev, int type, void *data,

int hci_recv_fragment(struct hci_dev *hdev, int type, void *data, int count)
{
+ int remaining = 0;
+ int err = 0;
+
if (type < HCI_ACLDATA_PKT || type > HCI_EVENT_PKT)
return -EILSEQ;

- while (count) {
+ do {
struct sk_buff *skb = __reassembly(hdev, type);
- struct { int expect; } *scb;
- int len = 0;
-
- if (!skb) {
- /* Start of the frame */
-
- switch (type) {
- case HCI_EVENT_PKT:
- if (count >= HCI_EVENT_HDR_SIZE) {
- struct hci_event_hdr *h = data;
- len = HCI_EVENT_HDR_SIZE + h->plen;
- } else
- return -EILSEQ;
- break;

- case HCI_ACLDATA_PKT:
- if (count >= HCI_ACL_HDR_SIZE) {
- struct hci_acl_hdr *h = data;
- len = HCI_ACL_HDR_SIZE + __le16_to_cpu(h->dlen);
- } else
- return -EILSEQ;
- break;
+ err = hci_reassembly(hdev, type, data, count, &skb, &remaining);

- case HCI_SCODATA_PKT:
- if (count >= HCI_SCO_HDR_SIZE) {
- struct hci_sco_hdr *h = data;
- len = HCI_SCO_HDR_SIZE + h->dlen;
- } else
- return -EILSEQ;
- break;
- }
+ if (err < 0)
+ return err;

- skb = bt_skb_alloc(len, GFP_ATOMIC);
- if (!skb) {
- BT_ERR("%s no memory for packet", hdev->name);
- return -ENOMEM;
- }
-
- skb->dev = (void *) hdev;
- bt_cb(skb)->pkt_type = type;
+ __reassembly(hdev, type) = skb;

- __reassembly(hdev, type) = skb;
-
- scb = (void *) skb->cb;
- scb->expect = len;
- } else {
- /* Continuation */
-
- scb = (void *) skb->cb;
- len = scb->expect;
- }
+ data += (count - remaining);
+ count = remaining;

- len = min(len, count);
+ } while (count);

- memcpy(skb_put(skb, len), data, len);
-
- scb->expect -= len;
-
- if (scb->expect == 0) {
- /* Complete frame */
-
- __reassembly(hdev, type) = NULL;
-
- bt_cb(skb)->pkt_type = type;
- hci_recv_frame(skb);
- }
-
- count -= len; data += len;
- }
-
- return 0;
+ return err;
}
EXPORT_SYMBOL(hci_recv_fragment);

--
1.7.0.4