Hi Suraj,
* Marcel Holtmann <marcel@xxxxxxxxxxxx> [2010-06-02 08:02:35 -0700]:
> Hi Suraj,
>
> > Implemented hci_recv_stream_fragment to reassemble HCI packets received from a data stream.
> >
> > Signed-off-by: suraj <suraj@xxxxxxxxxxx>
>
> please fix your signed-off-by line. This is not proper.
>
> > ---
> > include/net/bluetooth/hci_core.h | 1 +
> > net/bluetooth/hci_core.c | 98 ++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 99 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index e42f6ed..6f33f11 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -428,6 +428,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 5e83f8e..ac9ccf7 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -1033,6 +1033,104 @@ EXPORT_SYMBOL(hci_recv_frame);
> > /* Receive packet type fragment */
> > #define __reassembly(hdev, type) ((hdev)->reassembly[(type) - 2])
> >
> > +#define __get_max_rx_size(type) \
> > + (((type) == HCI_ACLDATA_PKT) ? \
> > + HCI_MAX_FRAME_SIZE : \
> > + ((type) == HCI_EVENT_PKT) ? HCI_MAX_EVENT_SIZE :\
> > + HCI_MAX_SCO_SIZE)
> > +
> > +#define __get_header_len(type) \
> > + (((type) == HCI_ACLDATA_PKT) ? \
> > + HCI_ACL_HDR_SIZE : \
> > + ((type) == HCI_EVENT_PKT) ? HCI_EVENT_HDR_SIZE :\
> > + HCI_SCO_HDR_SIZE)
>
> This is total hackish code. Who do you think is able to read this?
A switch sounds a way better for both macros, change that to a function
and use switch to compare.
>
> > +int hci_recv_stream_fragment(struct hci_dev *hdev, void *data, int count)
> > +{
> > + int type;
> > +
> > + while (count) {
> > + struct sk_buff *skb = __reassembly(hdev, HCI_ACLDATA_PKT);
> > +
> > + struct { int expect; int pkt_type; } *scb;
> > + int len = 0;
> > +
> > + if (!skb) {
> > + struct { char type; } *pkt;
> > +
> > + /* Start of the frame */
> > + pkt = data;
> > + type = pkt->type;
> > +
> > + if (type < HCI_ACLDATA_PKT || type > HCI_EVENT_PKT)
> > + return -EILSEQ;
> > +
> > + len = __get_max_rx_size(type);
> > +
> > + skb = bt_skb_alloc(len, GFP_ATOMIC);
> > + if (!skb)
> > + return -ENOMEM;
> > +
> > + scb = (void *) skb->cb;
> > + scb->expect = __get_header_len(type);
> > + scb->pkt_type = type;
> > +
> > + skb->dev = (void *) hdev;
> > + __reassembly(hdev, HCI_ACLDATA_PKT) = skb;
> > +
> > + data++;
> > + count--;
> > +
> > + continue;
> > + } else {
> > + scb = (void *) skb->cb;
> > + len = min(scb->expect, count);
> > + type = scb->pkt_type;
> > +
> > + memcpy(skb_put(skb, len), data, len);
> > +
> > + count -= len;
> > + data += len;
> > + scb->expect -= len;
> > + }
> > +
> > + 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;
> > + }
> > + 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);
> > + }
> > + 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;
> > + }
> > + break;
> > + }
> > +
> > + if (scb->expect == 0) {
> > + /* Complete frame */
> > +
> > + __reassembly(hdev, HCI_ACLDATA_PKT) = NULL;
> > +
> > + bt_cb(skb)->pkt_type = type;
> > + hci_recv_frame(skb);
> > + }
> > +
> > + }
> > + return 0;
> > +}
>
> I don't like this implementation at all. The biggest problem is that you
> are misusing __reassembly(hdev, HCI_ACLDATA_PKT) for getting your SKB. I
> don't wanna intermix this. I am also missing checks for the packet
> length matching or when packets are too big or the header size is not
> matching up.
>
> So in theory both functions do exactly the same. Only minor exception is
> that one knows the packet type up-front, the other has to read it from
> the stream as a 1-byte header. I don't wanna maintain two functions that
> do exactly the same.
>
> Creating an internal helper function that can maintain the current state
> of the reassembly sounds a lot better. Then re-use that function and
> ensure that the reassembly logic is inside the helper.
>
> Regards
>
> Marcel
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Gustavo F. Padovan
http://padovan.org
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
