Return-Path: Date: Wed, 2 Jun 2010 13:11:42 -0300 From: "Gustavo F. Padovan" To: suraj Cc: Marcel Holtmann , linux-bluetooth@vger.kernel.org, Luis.Rodriguez@Atheros.com, Jothikumar.Mothilal@Atheros.com Subject: Re: [PATCH v2] frame reassembly implementation for data stream Message-ID: <20100602161117.GA16657@vigoh> References: <1275467080.13946.3.camel@atheros013-desktop> <1275490955.2182.21.camel@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <1275490955.2182.21.camel@localhost.localdomain> Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Suraj, * Marcel Holtmann [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 > > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Gustavo F. Padovan http://padovan.org