Re: [PATCH v2] frame reassembly implementation for data stream

["Gustavo F. Padovan" <gustavo@padovan.org>]

Hi Suraj,

* Marcel Holtmann <marcel@holtmann.org> [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@Atheros.com>
> 
> 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