[PATCH bluetooth-next 0/5] Move skb delivery out of IPHC.

[PATCH bluetooth-next 0/5] Move skb delivery out of IPHC.

[Martin Townsend]

This series moves skb delivery out of IPHC and into the receive routines of
both bluetooth and 802.15.4.  The reason is that we need to support more
(de)compression schemes in the future.  It also means that calling 
lowpan_process_data now only returns error codes or 0 for success so 
this has been cleaned up.  The final patch then renames occurences of
lowpan_process_data and process_data to something more meaningful.

Martin Townsend (5):
  Remove skb_deliver from IPHC.
  Fix process_data return values
  Use consume_skb when packet processed successfully.
  Remove unused skb_delivery_cb typedef.
  Rename process_data and lowpan_process_data.

 include/net/6lowpan.h         | 12 ++++++------
 net/6lowpan/iphc.c            | 42 ++++++++++++------------------------------
 net/bluetooth/6lowpan.c       | 29 ++++++++++++++++++++---------
 net/ieee802154/6lowpan_rtnl.c | 34 +++++++++++++++-------------------
 4 files changed, 53 insertions(+), 64 deletions(-)

-- 
1.9.1

[PATCH bluetooth-next 1/5] Remove skb_deliver from IPHC.

[Martin Townsend]

Separating skb delivery from decompression ensures that we can support further
decompression schemes and removes the mixed return value of error codes with
NET_RX_FOO.

Signed-off-by: Martin Townsend <mtownsend1973@gmail.com>
---
 include/net/6lowpan.h         |  2 +-
 net/6lowpan/iphc.c            | 32 ++++++--------------------------
 net/bluetooth/6lowpan.c       | 12 +++++++++++-
 net/ieee802154/6lowpan_rtnl.c | 15 +++++----------
 4 files changed, 23 insertions(+), 38 deletions(-)

diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
index d184df1..99aa7e3 100644
--- a/include/net/6lowpan.h
+++ b/include/net/6lowpan.h
@@ -377,7 +377,7 @@ typedef int (*skb_delivery_cb)(struct sk_buff *skb, struct net_device *dev);
 int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 		const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
 		const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
-		u8 iphc0, u8 iphc1, skb_delivery_cb skb_deliver);
+		u8 iphc0, u8 iphc1);
 int lowpan_header_compress(struct sk_buff *skb, struct net_device *dev,
 			unsigned short type, const void *_daddr,
 			const void *_saddr, unsigned int len);
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 747b3cc..45714fe 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -171,29 +171,6 @@ static int uncompress_context_based_src_addr(struct sk_buff *skb,
 	return 0;
 }
 
-static int skb_deliver(struct sk_buff *skb, struct ipv6hdr *hdr,
-		       struct net_device *dev, skb_delivery_cb deliver_skb)
-{
-	int stat;
-
-	skb_push(skb, sizeof(struct ipv6hdr));
-	skb_reset_network_header(skb);
-	skb_copy_to_linear_data(skb, hdr, sizeof(struct ipv6hdr));
-
-	skb->protocol = htons(ETH_P_IPV6);
-	skb->pkt_type = PACKET_HOST;
-	skb->dev = dev;
-
-	raw_dump_table(__func__, "raw skb data dump before receiving",
-		       skb->data, skb->len);
-
-	stat = deliver_skb(skb, dev);
-
-	consume_skb(skb);
-
-	return stat;
-}
-
 /* Uncompress function for multicast destination address,
  * when M bit is set.
  */
@@ -327,7 +304,7 @@ static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
 int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 			const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
 			const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
-			u8 iphc0, u8 iphc1, skb_delivery_cb deliver_skb)
+			u8 iphc0, u8 iphc1)
 {
 	struct ipv6hdr hdr = {};
 	u8 tmp, num_context = 0;
@@ -492,10 +469,13 @@ int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 		hdr.version, ntohs(hdr.payload_len), hdr.nexthdr,
 		hdr.hop_limit, &hdr.daddr);
 
-	raw_dump_table(__func__, "raw header dump", (u8 *)&hdr, sizeof(hdr));
+	skb_push(skb, sizeof(hdr));
+	skb_reset_network_header(skb);
+	skb_copy_to_linear_data(skb, &hdr, sizeof(hdr));
 
-	return skb_deliver(skb, &hdr, dev, deliver_skb);
+	raw_dump_table(__func__, "raw header dump", (u8 *)&hdr, sizeof(hdr));
 
+	return 0;
 drop:
 	kfree_skb(skb);
 	return -EINVAL;
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 6c5c2ef..03787e0 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -290,7 +290,7 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
 	return lowpan_process_data(skb, netdev,
 				   saddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
 				   daddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
-				   iphc0, iphc1, give_skb_to_upper);
+				   iphc0, iphc1);
 
 drop:
 	kfree_skb(skb);
@@ -350,6 +350,16 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
 			if (ret != NET_RX_SUCCESS)
 				goto drop;
 
+			local_skb->protocol = htons(ETH_P_IPV6);
+			local_skb->pkt_type = PACKET_HOST;
+			local_skb->dev = dev;
+
+			if (give_skb_to_upper(local_skb, dev)
+					!= NET_RX_SUCCESS) {
+				kfree_skb(local_skb);
+				goto drop;
+			}
+
 			dev->stats.rx_bytes += skb->len;
 			dev->stats.rx_packets++;
 
diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
index 0c1a49b..82ef0df 100644
--- a/net/ieee802154/6lowpan_rtnl.c
+++ b/net/ieee802154/6lowpan_rtnl.c
@@ -190,8 +190,7 @@ static int process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
 
 	return lowpan_process_data(skb, skb->dev, sap, sa.addr_type,
 				   IEEE802154_ADDR_LEN, dap, da.addr_type,
-				   IEEE802154_ADDR_LEN, iphc0, iphc1,
-				   lowpan_give_skb_to_devices);
+				   IEEE802154_ADDR_LEN, iphc0, iphc1);
 
 drop:
 	kfree_skb(skb);
@@ -528,15 +527,8 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	/* check that it's our buffer */
 	if (skb->data[0] == LOWPAN_DISPATCH_IPV6) {
-		skb->protocol = htons(ETH_P_IPV6);
-		skb->pkt_type = PACKET_HOST;
-
 		/* Pull off the 1-byte of 6lowpan header. */
 		skb_pull(skb, 1);
-
-		ret = lowpan_give_skb_to_devices(skb, NULL);
-		if (ret == NET_RX_DROP)
-			goto drop;
 	} else {
 		switch (skb->data[0] & 0xe0) {
 		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
@@ -565,7 +557,10 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
 		}
 	}
 
-	return NET_RX_SUCCESS;
+	/* Pass IPv6 packet up to the next layer */
+	skb->protocol = htons(ETH_P_IPV6);
+	skb->pkt_type = PACKET_HOST;
+	return lowpan_give_skb_to_devices(skb, NULL);
 drop_skb:
 	kfree_skb(skb);
 drop:
-- 
1.9.1

[PATCH bluetooth-next 2/5] Fix process_data return values

[Martin Townsend]

As process_data now returns just error codes fix up the calls to this
function to only drop the skb if an error code is returned.

Signed-off-by: Martin Townsend <mtownsend1973@gmail.com>
---
 net/bluetooth/6lowpan.c       | 2 +-
 net/ieee802154/6lowpan_rtnl.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 03787e0..702bf3c 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -347,7 +347,7 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
 				goto drop;
 
 			ret = process_data(local_skb, dev, chan);
-			if (ret != NET_RX_SUCCESS)
+			if (ret < 0)
 				goto drop;
 
 			local_skb->protocol = htons(ETH_P_IPV6);
diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
index 82ef0df..2c6bc36 100644
--- a/net/ieee802154/6lowpan_rtnl.c
+++ b/net/ieee802154/6lowpan_rtnl.c
@@ -533,14 +533,14 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
 		switch (skb->data[0] & 0xe0) {
 		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
 			ret = process_data(skb, &hdr);
-			if (ret == NET_RX_DROP)
+			if (ret < 0)
 				goto drop;
 			break;
 		case LOWPAN_DISPATCH_FRAG1:	/* first fragment header */
 			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAG1);
 			if (ret == 1) {
 				ret = process_data(skb, &hdr);
-				if (ret == NET_RX_DROP)
+				if (ret < 0)
 					goto drop;
 			}
 			break;
@@ -548,7 +548,7 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
 			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
 			if (ret == 1) {
 				ret = process_data(skb, &hdr);
-				if (ret == NET_RX_DROP)
+				if (ret < 0)
 					goto drop;
 			}
 			break;
-- 
1.9.1

[PATCH bluetooth-next 3/5] Use consume_skb when packet processed successfully.

[Martin Townsend]

Signed-off-by: Martin Townsend <mtownsend1973@gmail.com>
---
 net/bluetooth/6lowpan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index 702bf3c..ec68706 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -363,7 +363,7 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
 			dev->stats.rx_bytes += skb->len;
 			dev->stats.rx_packets++;
 
-			kfree_skb(skb);
+			consume_skb(skb);
 			break;
 		default:
 			break;
-- 
1.9.1

[PATCH bluetooth-next 4/5] Remove unused skb_delivery_cb typedef.

[Martin Townsend]

Signed-off-by: Martin Townsend <mtownsend1973@gmail.com>
---
 include/net/6lowpan.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
index 99aa7e3..abfa359 100644
--- a/include/net/6lowpan.h
+++ b/include/net/6lowpan.h
@@ -372,8 +372,6 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16 *dgram_offset)
 	return skb->len + uncomp_header - ret;
 }
 
-typedef int (*skb_delivery_cb)(struct sk_buff *skb, struct net_device *dev);
-
 int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
 		const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
 		const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
-- 
1.9.1

[PATCH bluetooth-next 5/5] Rename process_data and lowpan_process_data.

[Martin Townsend]

As we have decouple decompression from data delivery we can now rename all
occurences of process_data in receive path.

Signed-off-by: Martin Townsend <mtownsend1973@gmail.com>
---
 include/net/6lowpan.h         | 10 ++++++----
 net/6lowpan/iphc.c            | 12 +++++++-----
 net/bluetooth/6lowpan.c       | 15 ++++++++-------
 net/ieee802154/6lowpan_rtnl.c | 15 ++++++++-------
 4 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
index abfa359..dc03d77 100644
--- a/include/net/6lowpan.h
+++ b/include/net/6lowpan.h
@@ -372,10 +372,12 @@ lowpan_uncompress_size(const struct sk_buff *skb, u16 *dgram_offset)
 	return skb->len + uncomp_header - ret;
 }
 
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
-		const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
-		const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
-		u8 iphc0, u8 iphc1);
+int
+lowpan_header_decompress(struct sk_buff *skb, struct net_device *dev,
+			 const u8 *saddr, const u8 saddr_type,
+			 const u8 saddr_len, const u8 *daddr,
+			 const u8 daddr_type, const u8 daddr_len,
+			 u8 iphc0, u8 iphc1);
 int lowpan_header_compress(struct sk_buff *skb, struct net_device *dev,
 			unsigned short type, const void *_daddr,
 			const void *_saddr, unsigned int len);
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 45714fe..73a7065 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -301,10 +301,12 @@ err:
 /* TTL uncompression values */
 static const u8 lowpan_ttl_values[] = { 0, 1, 64, 255 };
 
-int lowpan_process_data(struct sk_buff *skb, struct net_device *dev,
-			const u8 *saddr, const u8 saddr_type, const u8 saddr_len,
-			const u8 *daddr, const u8 daddr_type, const u8 daddr_len,
-			u8 iphc0, u8 iphc1)
+int
+lowpan_header_decompress(struct sk_buff *skb, struct net_device *dev,
+			 const u8 *saddr, const u8 saddr_type,
+			 const u8 saddr_len, const u8 *daddr,
+			 const u8 daddr_type, const u8 daddr_len,
+			 u8 iphc0, u8 iphc1)
 {
 	struct ipv6hdr hdr = {};
 	u8 tmp, num_context = 0;
@@ -480,7 +482,7 @@ drop:
 	kfree_skb(skb);
 	return -EINVAL;
 }
-EXPORT_SYMBOL_GPL(lowpan_process_data);
+EXPORT_SYMBOL_GPL(lowpan_header_decompress);
 
 static u8 lowpan_compress_addr_64(u8 **hc_ptr, u8 shift,
 				  const struct in6_addr *ipaddr,
diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
index ec68706..32630c8 100644
--- a/net/bluetooth/6lowpan.c
+++ b/net/bluetooth/6lowpan.c
@@ -257,8 +257,8 @@ static int give_skb_to_upper(struct sk_buff *skb, struct net_device *dev)
 	return netif_rx(skb_cp);
 }
 
-static int process_data(struct sk_buff *skb, struct net_device *netdev,
-			struct l2cap_chan *chan)
+static int iphc_decompress(struct sk_buff *skb, struct net_device *netdev,
+			   struct l2cap_chan *chan)
 {
 	const u8 *saddr, *daddr;
 	u8 iphc0, iphc1;
@@ -287,10 +287,11 @@ static int process_data(struct sk_buff *skb, struct net_device *netdev,
 	if (lowpan_fetch_skb_u8(skb, &iphc1))
 		goto drop;
 
-	return lowpan_process_data(skb, netdev,
-				   saddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
-				   daddr, IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
-				   iphc0, iphc1);
+	return lowpan_header_decompress(skb, netdev,
+					saddr, IEEE802154_ADDR_LONG,
+					EUI64_ADDR_LEN, daddr,
+					IEEE802154_ADDR_LONG, EUI64_ADDR_LEN,
+					iphc0, iphc1);
 
 drop:
 	kfree_skb(skb);
@@ -346,7 +347,7 @@ static int recv_pkt(struct sk_buff *skb, struct net_device *dev,
 			if (!local_skb)
 				goto drop;
 
-			ret = process_data(local_skb, dev, chan);
+			ret = iphc_decompress(local_skb, dev, chan);
 			if (ret < 0)
 				goto drop;
 
diff --git a/net/ieee802154/6lowpan_rtnl.c b/net/ieee802154/6lowpan_rtnl.c
index 2c6bc36..f33ec10 100644
--- a/net/ieee802154/6lowpan_rtnl.c
+++ b/net/ieee802154/6lowpan_rtnl.c
@@ -158,7 +158,8 @@ static int lowpan_give_skb_to_devices(struct sk_buff *skb,
 	return stat;
 }
 
-static int process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
+static int
+iphc_decompress(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
 {
 	u8 iphc0, iphc1;
 	struct ieee802154_addr_sa sa, da;
@@ -188,9 +189,9 @@ static int process_data(struct sk_buff *skb, const struct ieee802154_hdr *hdr)
 	else
 		dap = &da.hwaddr;
 
-	return lowpan_process_data(skb, skb->dev, sap, sa.addr_type,
-				   IEEE802154_ADDR_LEN, dap, da.addr_type,
-				   IEEE802154_ADDR_LEN, iphc0, iphc1);
+	return lowpan_header_decompress(skb, skb->dev, sap, sa.addr_type,
+					IEEE802154_ADDR_LEN, dap, da.addr_type,
+					IEEE802154_ADDR_LEN, iphc0, iphc1);
 
 drop:
 	kfree_skb(skb);
@@ -532,14 +533,14 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
 	} else {
 		switch (skb->data[0] & 0xe0) {
 		case LOWPAN_DISPATCH_IPHC:	/* ipv6 datagram */
-			ret = process_data(skb, &hdr);
+			ret = iphc_decompress(skb, &hdr);
 			if (ret < 0)
 				goto drop;
 			break;
 		case LOWPAN_DISPATCH_FRAG1:	/* first fragment header */
 			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAG1);
 			if (ret == 1) {
-				ret = process_data(skb, &hdr);
+				ret = iphc_decompress(skb, &hdr);
 				if (ret < 0)
 					goto drop;
 			}
@@ -547,7 +548,7 @@ static int lowpan_rcv(struct sk_buff *skb, struct net_device *dev,
 		case LOWPAN_DISPATCH_FRAGN:	/* next fragments headers */
 			ret = lowpan_frag_rcv(skb, LOWPAN_DISPATCH_FRAGN);
 			if (ret == 1) {
-				ret = process_data(skb, &hdr);
+				ret = iphc_decompress(skb, &hdr);
 				if (ret < 0)
 					goto drop;
 			}
-- 
1.9.1

Re: [PATCH bluetooth-next 0/5] Move skb delivery out of IPHC.

[Alexander Aring]

Hi Martin,

I did a fast quick test and it's still working. I see now the conversion
from to errno, but you didn't change/fix the behaviour with kfree_skb. I
am pretty sure this is still broken with and without your patch.

I know you want to make smaller patches but can you please add the
conversion simple to this patch series? What I see now is the change
from if (ret != NET_RX_FOO) to errno. This change the behaviour of
current somewhat working state. What I mean, it's okay to change that
but please in a serie which contains also fixes for the kfree_skb thing.

Important:
All patches should be compileable after each patch keyword "bisectable"
otherwise people can't use "git bisect".

But in your case this should be fine.

It should also not broken between them, but this is a bigger issue.
Smaller patches for better review are welcome.


To your patch series:

- You missed a tag "6lowpan:", "ieee802154: 6lowpan:" or "bluetooth: 6lowpan:"
  Use the tag which best fit to your patch. Also add a tag to the
  cover-letter "6lowpan:"

- Please "fixup" patch 4/5 with patch 1/5. "fixup" is some git language,
  what I mean is just include the remove of "skb_delivery_cb skb_deliver", when
  remove the use of that. Now you have two patches for this in your git
  history, with "git rebase -i" and a fixup you don't need to touch the
  patch again. Just for information, maybe you don't know that. Fast
  googleing results [0] tutorial.

- Sometimes you add a dot in commit message, sometimes not. Please start
  the commit msg in lower case and remove the dot.


- Alex

[0] http://fle.github.io/git-tip-keep-your-branch-clean-with-fixup-and-autosquash.html

Re: [PATCH bluetooth-next 0/5] Move skb delivery out of IPHC.

[Martin Townsend]

Hi Alex,

On 20/10/14 08:25, Alexander Aring wrote:
> Hi Martin,
>
> I did a fast quick test and it's still working. I see now the conversion
> from to errno, but you didn't change/fix the behaviour with kfree_skb. I
> am pretty sure this is still broken with and without your patch.
>
> I know you want to make smaller patches but can you please add the
> conversion simple to this patch series? What I see now is the change
> from if (ret != NET_RX_FOO) to errno. This change the behaviour of
> current somewhat working state. What I mean, it's okay to change that
> but please in a serie which contains also fixes for the kfree_skb thing.
Is this the kfree_skb in lowpan_give_skb_to_devices? I'll add it in.
> Important:
> All patches should be compileable after each patch keyword "bisectable"
> otherwise people can't use "git bisect".
After each patch everything should be compilable or at least I thought is was :)
> But in your case this should be fine.
>
> It should also not broken between them, but this is a bigger issue.
> Smaller patches for better review are welcome.
>
>
> To your patch series:
>
> - You missed a tag "6lowpan:", "ieee802154: 6lowpan:" or "bluetooth: 6lowpan:"
>   Use the tag which best fit to your patch. Also add a tag to the
>   cover-letter "6lowpan:"
will do
>
> - Please "fixup" patch 4/5 with patch 1/5. "fixup" is some git language,
>   what I mean is just include the remove of "skb_delivery_cb skb_deliver", when
>   remove the use of that. Now you have two patches for this in your git
>   history, with "git rebase -i" and a fixup you don't need to touch the
>   patch again. Just for information, maybe you don't know that. Fast
>   googleing results [0] tutorial.
no probs
>
> - Sometimes you add a dot in commit message, sometimes not. Please start
>   the commit msg in lower case and remove the dot.
>
ok.
> - Alex
>
> [0] http://fle.github.io/git-tip-keep-your-branch-clean-with-fixup-and-autosquash.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

- Martin.

Re: [PATCH bluetooth-next 0/5] Move skb delivery out of IPHC.

[Alexander Aring]

Hi Martin,

On Mon, Oct 20, 2014 at 09:34:21AM +0100, Martin Townsend wrote:
> Hi Alex,
> 
> On 20/10/14 08:25, Alexander Aring wrote:
> > Hi Martin,
> >
> > I did a fast quick test and it's still working. I see now the conversion
> > from to errno, but you didn't change/fix the behaviour with kfree_skb. I
> > am pretty sure this is still broken with and without your patch.
> >
> > I know you want to make smaller patches but can you please add the
> > conversion simple to this patch series? What I see now is the change
> > from if (ret != NET_RX_FOO) to errno. This change the behaviour of
> > current somewhat working state. What I mean, it's okay to change that
> > but please in a serie which contains also fixes for the kfree_skb thing.
> Is this the kfree_skb in lowpan_give_skb_to_devices? I'll add it in.

mhh no, I thought you main goal was to move out the error handling of
lowpan_process_header function (don't know the new name, maybe it is
iphc_decompression).

- Alex

Re: [PATCH bluetooth-next 0/5] Move skb delivery out of IPHC.

[Martin Townsend]

Hi Alex,

I've completely forgotten what the original problem was it was so long ago :)  , let me dig through the old emails and see if I can jog my memory. 

My intention was to do the kfree_skb patch in my next submission but I'll see what I can do.

- Martin.



On 20/10/14 09:55, Alexander Aring wrote:
> Hi Martin,
>
> On Mon, Oct 20, 2014 at 09:34:21AM +0100, Martin Townsend wrote:
>> Hi Alex,
>>
>> On 20/10/14 08:25, Alexander Aring wrote:
>>> Hi Martin,
>>>
>>> I did a fast quick test and it's still working. I see now the conversion
>>> from to errno, but you didn't change/fix the behaviour with kfree_skb. I
>>> am pretty sure this is still broken with and without your patch.
>>>
>>> I know you want to make smaller patches but can you please add the
>>> conversion simple to this patch series? What I see now is the change
>>> from if (ret != NET_RX_FOO) to errno. This change the behaviour of
>>> current somewhat working state. What I mean, it's okay to change that
>>> but please in a serie which contains also fixes for the kfree_skb thing.
>> Is this the kfree_skb in lowpan_give_skb_to_devices? I'll add it in.
> mhh no, I thought you main goal was to move out the error handling of
> lowpan_process_header function (don't know the new name, maybe it is
> iphc_decompression).
>
> - Alex

Re: [PATCH bluetooth-next 0/5] Move skb delivery out of IPHC.

[Alexander Aring]

On Mon, Oct 20, 2014 at 10:02:17AM +0100, Martin Townsend wrote:
> Hi Alex,
> 
> I've completely forgotten what the original problem was it was so long ago :)  , let me dig through the old emails and see if I can jog my memory. 
> 
> My intention was to do the kfree_skb patch in my next submission but I'll see what I can do.
> 

yea, the problem a general error handling issue in mainly two parts:

 - detection errors. Means the NET_RX_FOO conversion to errno. But you
   put out the netif_rx call outside of lowpan_header_create which is
   very well. I approve that one. Now it's important that the "lowpan
   give to upper layer" functions not returning ERRNO's and NET_RX_FOO.

   Again, errno is a negative value and NET_RX_FOO is 0 or 1. We can't
   check on a error with if (ret < 0) or (ret == NET_RX_DROP). Also
   (ret != NET_RX_SUCCESS) will not work because it's "1" and "0"
   indicate successful or sometimes not successful because NET_RX_DROP
   was returned and this is "0". Lot of confusing here.

   For the "lowpan give to upper layer" functions doesn't matter if
   errno is returned or NET_RX_FOO, but don't a mix of both!
   The "lowpan_header_create" should return errno's.

 - Second issue was a complete wrong reaction on error handling and
   memory managment with "kfree_skb" which needs a fix of the above one
   at first to introduce a correct error handling.


Now I see a little bit the fix of the first one. But not the second one.
Or should these things all fine now, otherwise I will take a more
careful review.

- Alex

Re: [PATCH bluetooth-next 0/5] Move skb delivery out of IPHC.

[Martin Townsend]

Hi Alex,

I think if I put the skb free stuff in lowpan_give_skb_to_devices then it should all be fixed.

- lowpan_process_data or now lowpan_header_decompress will either
     - always free the skb on error and return an error code
     - or return 0.  So there is no chance of NET_RX_FOO codes being returned as we have moved skb deilvery out of this path.

- This means that lowpan_recv can now checks to see if ret < 0, I could change this to ret != 0 if you want but the outcome is the same, if decompression fails then skb has been freed so goto drop.

- This leaves the last part lowpan_give_skb_to_devices must now free or consume the skb (which I've just added).

Hopefully this sorts out the original problem? or have I missed something.

My next patch series will remove all the kfree_skb's from decompression but I don't think this fixes anything just makes the code more maintainable in that we don't have to worry about making sure all error paths are covered.

Let me know what you think and then I'll send v2.

- Martin.



On 20/10/14 10:10, Alexander Aring wrote:
> On Mon, Oct 20, 2014 at 10:02:17AM +0100, Martin Townsend wrote:
>> Hi Alex,
>>
>> I've completely forgotten what the original problem was it was so long ago :)  , let me dig through the old emails and see if I can jog my memory. 
>>
>> My intention was to do the kfree_skb patch in my next submission but I'll see what I can do.
>>
> yea, the problem a general error handling issue in mainly two parts:
>
>  - detection errors. Means the NET_RX_FOO conversion to errno. But you
>    put out the netif_rx call outside of lowpan_header_create which is
>    very well. I approve that one. Now it's important that the "lowpan
>    give to upper layer" functions not returning ERRNO's and NET_RX_FOO.
>
>    Again, errno is a negative value and NET_RX_FOO is 0 or 1. We can't
>    check on a error with if (ret < 0) or (ret == NET_RX_DROP). Also
>    (ret != NET_RX_SUCCESS) will not work because it's "1" and "0"
>    indicate successful or sometimes not successful because NET_RX_DROP
>    was returned and this is "0". Lot of confusing here.
>
>    For the "lowpan give to upper layer" functions doesn't matter if
>    errno is returned or NET_RX_FOO, but don't a mix of both!
>    The "lowpan_header_create" should return errno's.
>
>  - Second issue was a complete wrong reaction on error handling and
>    memory managment with "kfree_skb" which needs a fix of the above one
>    at first to introduce a correct error handling.
>
>
> Now I see a little bit the fix of the first one. But not the second one.
> Or should these things all fine now, otherwise I will take a more
> careful review.
>
> - Alex
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html