| Message ID | 1350965397-12384-7-git-send-email-tony.cheneau@amnesiak.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 10/23/2012 06:09 AM, Tony Cheneau wrote: > The first fragment, FRAG1, must contain some payload according to the > specs. However, as it is currently written, the first fragment will > remain empty and only contain the 6lowpan headers. > > This patch also extract the transport layer information from the first > fragment. This information is later on use when uncompressing UDP > header. > > Signed-off-by: Tony Cheneau <tony.cheneau@amnesiak.org> > --- > net/ieee802154/6lowpan.c | 54 +++++++++++++++++++++++++++++++++++---------- > 1 files changed, 42 insertions(+), 12 deletions(-) > > diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c > index 8a2ee95..38cecaf 100644 > --- a/net/ieee802154/6lowpan.c > +++ b/net/ieee802154/6lowpan.c > @@ -654,7 +654,7 @@ static void lowpan_fragment_timer_expired(unsigned long entry_addr) > } > > static struct lowpan_fragment * > -lowpan_alloc_new_frame(struct sk_buff *skb, u8 len, u16 tag) > +lowpan_alloc_new_frame(struct sk_buff *skb, u16 len, u16 tag) > { > struct lowpan_fragment *frame; > > @@ -735,6 +735,18 @@ lowpan_process_data(struct sk_buff *skb) > /* adds the 3 MSB to the 8 LSB to retrieve the 11 bits length */ > len = ((iphc0 & 7) << 8) | slen; > > + if ((iphc0 & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAG1) { > + pr_debug("%s received a FRAG1 packet (tag: %d, " > + "size of the entire IP packet: %d)" > + , __func__, tag, len); There are several schools of thought on the relative importance of observing the 80-character line limit versus breaking up string constants (in an attempt to maintain grepability). I think the above is fine but others (whose opinion matters more than mine) may or may not agree. Whatever you decide here, please apply consistently throughout. However, the comma ahead of the __func__ should be at the end of the previous line. (...) > - /* if payload length is zero, therefore it's a first fragment */ > - hlen = (plen == 0 ? LOWPAN_FRAG1_HEAD_SIZE : LOWPAN_FRAGN_HEAD_SIZE); > + hlen = (type == LOWPAN_DISPATCH_FRAG1 ? LOWPAN_FRAG1_HEAD_SIZE : > + LOWPAN_FRAGN_HEAD_SIZE); The second line of this statement should be aligned as follows: + hlen = (type == LOWPAN_DISPATCH_FRAG1 ? LOWPAN_FRAG1_HEAD_SIZE : + LOWPAN_FRAGN_HEAD_SIZE); So the L for LOWPAN_FRAGN_HEAD_SIZE should be underneath the t for type. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Jan, Thank you for all your comments. See my answer inline. Le 23.10.2012 09:19, Jan Ceuleers a écrit : > On 10/23/2012 06:09 AM, Tony Cheneau wrote: >> The first fragment, FRAG1, must contain some payload according to >> the >> specs. However, as it is currently written, the first fragment will >> remain empty and only contain the 6lowpan headers. >> >> This patch also extract the transport layer information from the >> first >> fragment. This information is later on use when uncompressing UDP >> header. >> >> Signed-off-by: Tony Cheneau <tony.cheneau@amnesiak.org> >> --- >> net/ieee802154/6lowpan.c | 54 >> +++++++++++++++++++++++++++++++++++---------- >> 1 files changed, 42 insertions(+), 12 deletions(-) >> >> diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c >> index 8a2ee95..38cecaf 100644 >> --- a/net/ieee802154/6lowpan.c >> +++ b/net/ieee802154/6lowpan.c >> @@ -654,7 +654,7 @@ static void >> lowpan_fragment_timer_expired(unsigned long entry_addr) >> } >> >> static struct lowpan_fragment * >> -lowpan_alloc_new_frame(struct sk_buff *skb, u8 len, u16 tag) >> +lowpan_alloc_new_frame(struct sk_buff *skb, u16 len, u16 tag) >> { >> struct lowpan_fragment *frame; >> >> @@ -735,6 +735,18 @@ lowpan_process_data(struct sk_buff *skb) >> /* adds the 3 MSB to the 8 LSB to retrieve the 11 bits length */ >> len = ((iphc0 & 7) << 8) | slen; >> >> + if ((iphc0 & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAG1) { >> + pr_debug("%s received a FRAG1 packet (tag: %d, " >> + "size of the entire IP packet: %d)" >> + , __func__, tag, len); > > There are several schools of thought on the relative importance of > observing the 80-character line limit versus breaking up string > constants (in an attempt to maintain grepability). I think the above > is > fine but others (whose opinion matters more than mine) may or may not > agree. Whatever you decide here, please apply consistently > throughout. Yes, I've seen that particular issues when running checkpatch.pl. I decided to break down line, but I can easily be convinced to do things differently. Anyway, I'll make sure that all my patches are consistent in breaking up string after 80 characters the same way. > However, the comma ahead of the __func__ should be at the end of the > previous line. Will do. > >> - /* if payload length is zero, therefore it's a first fragment */ >> - hlen = (plen == 0 ? LOWPAN_FRAG1_HEAD_SIZE : >> LOWPAN_FRAGN_HEAD_SIZE); >> + hlen = (type == LOWPAN_DISPATCH_FRAG1 ? LOWPAN_FRAG1_HEAD_SIZE : >> + LOWPAN_FRAGN_HEAD_SIZE); > > The second line of this statement should be aligned as follows: > > + hlen = (type == LOWPAN_DISPATCH_FRAG1 ? LOWPAN_FRAG1_HEAD_SIZE : > + LOWPAN_FRAGN_HEAD_SIZE); > > So the L for LOWPAN_FRAGN_HEAD_SIZE should be underneath the t for > type. Will do as well. Again, thank you for all your detailed comments. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2012-10-23 at 16:50 +0200, Tony Cheneau wrote: > > > > + hlen = (type == LOWPAN_DISPATCH_FRAG1 ? LOWPAN_FRAG1_HEAD_SIZE : > > + LOWPAN_FRAGN_HEAD_SIZE); > > > > So the L for LOWPAN_FRAGN_HEAD_SIZE should be underneath the t for > > type. > Will do as well. By the way parens are not needed, or better like that : hlen = (type == LOWPAN_DISPATCH_FRAG1) ? LOWPAN_FRAG1_HEAD_SIZE : LOWPAN_FRAGN_HEAD_SIZE; -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c index 8a2ee95..38cecaf 100644 --- a/net/ieee802154/6lowpan.c +++ b/net/ieee802154/6lowpan.c @@ -654,7 +654,7 @@ static void lowpan_fragment_timer_expired(unsigned long entry_addr) } static struct lowpan_fragment * -lowpan_alloc_new_frame(struct sk_buff *skb, u8 len, u16 tag) +lowpan_alloc_new_frame(struct sk_buff *skb, u16 len, u16 tag) { struct lowpan_fragment *frame; @@ -735,6 +735,18 @@ lowpan_process_data(struct sk_buff *skb) /* adds the 3 MSB to the 8 LSB to retrieve the 11 bits length */ len = ((iphc0 & 7) << 8) | slen; + if ((iphc0 & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAG1) { + pr_debug("%s received a FRAG1 packet (tag: %d, " + "size of the entire IP packet: %d)" + , __func__, tag, len); + } else { /* FRAGN */ + if (lowpan_fetch_skb_u8(skb, &offset)) + goto unlock_and_drop; + pr_debug("%s received a FRAGN packet (tag: %d, " + "size of the entire IP packet: %d, " + "offset: %d)", __func__, tag, len, offset * 8); + } + /* * check if frame assembling with the same tag is * already in progress @@ -749,17 +761,13 @@ lowpan_process_data(struct sk_buff *skb) /* alloc new frame structure */ if (!found) { + pr_debug("%s first fragment received for tag %d, " + "begin packet reassembly", __func__, tag); frame = lowpan_alloc_new_frame(skb, len, tag); if (!frame) goto unlock_and_drop; } - if ((iphc0 & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAG1) - goto unlock_and_drop; - - if (lowpan_fetch_skb_u8(skb, &offset)) /* fetch offset */ - goto unlock_and_drop; - /* if payload fits buffer, copy it */ if (likely((offset * 8 + skb->len) <= frame->length)) skb_copy_to_linear_data_offset(frame->skb, offset * 8, @@ -777,6 +785,10 @@ lowpan_process_data(struct sk_buff *skb) list_del(&frame->list); spin_unlock_bh(&flist_lock); + pr_debug("%s successfully reassembled fragment " + "(tag %d)", __func__, tag); + + dev_kfree_skb(skb); skb = frame->skb; kfree(frame); @@ -976,13 +988,13 @@ static int lowpan_get_mac_header_length(struct sk_buff *skb) static int lowpan_fragment_xmit(struct sk_buff *skb, u8 *head, - int mlen, int plen, int offset) + int mlen, int plen, int offset, int type) { struct sk_buff *frag; int hlen, ret; - /* if payload length is zero, therefore it's a first fragment */ - hlen = (plen == 0 ? LOWPAN_FRAG1_HEAD_SIZE : LOWPAN_FRAGN_HEAD_SIZE); + hlen = (type == LOWPAN_DISPATCH_FRAG1 ? LOWPAN_FRAG1_HEAD_SIZE : + LOWPAN_FRAGN_HEAD_SIZE); lowpan_raw_dump_inline(__func__, "6lowpan fragment header", head, hlen); @@ -1025,7 +1037,18 @@ lowpan_skb_fragmentation(struct sk_buff *skb) head[2] = tag >> 8; head[3] = tag & 0xff; - err = lowpan_fragment_xmit(skb, head, header_length, 0, 0); + err = lowpan_fragment_xmit(skb, head, header_length, LOWPAN_FRAG_SIZE, + 0, LOWPAN_DISPATCH_FRAG1); + + if (err) { +#if DEBUG + pr_debug("%s unable to send FRAG1 packet (tag: %d)", + __func__, tag); +#endif /* DEBUG */ + goto exit; + } + + offset = LOWPAN_FRAG_SIZE; /* next fragment header */ head[0] &= ~LOWPAN_DISPATCH_FRAG1; @@ -1040,10 +1063,17 @@ lowpan_skb_fragmentation(struct sk_buff *skb) len = payload_length - offset; err = lowpan_fragment_xmit(skb, head, header_length, - len, offset); + len, offset, LOWPAN_DISPATCH_FRAGN); + if (err) { + pr_debug("%s unable to send a subsequent FRAGN packet " + "(tag: %d, offset: %d", __func__, tag, offset); + goto exit; + } + offset += len; } +exit: return err; }
The first fragment, FRAG1, must contain some payload according to the specs. However, as it is currently written, the first fragment will remain empty and only contain the 6lowpan headers. This patch also extract the transport layer information from the first fragment. This information is later on use when uncompressing UDP header. Signed-off-by: Tony Cheneau <tony.cheneau@amnesiak.org> --- net/ieee802154/6lowpan.c | 54 +++++++++++++++++++++++++++++++++++---------- 1 files changed, 42 insertions(+), 12 deletions(-)