Re: [RFCv2 bluetooth-next 3/3] 6lowpan: iphc: add support for stateful compression

[Alexander Aring <alex.aring-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hi,

On Wed, Nov 25, 2015 at 06:12:25PM +0100, Stefan Schmidt wrote:
> Helllo.
> 
> On 17/11/15 23:33, Alexander Aring wrote:
> >This patch introduce support for IPHC stateful address compression. It
> >will offer three debugfs per interface entries, which are:
> >
> >  - dci_table: destination context indentifier table
> >  - sci_table: source context indentifier table
> >  - mcast_sci_table: multicast context identifier table
> >
> >Example to setup a context id:
> >
> >A "cat /sys/kernel/debug/6lowpan/lowpan0/dci_table" will display all
> >contexts which are available. Example:
> >
> >ID ipv6-address/prefix-length                  state
> >0  0000:0000:0000:0000:0000:0000:0000:0000/000 0
> >1  0000:0000:0000:0000:0000:0000:0000:0000/000 0
> >2  0000:0000:0000:0000:0000:0000:0000:0000/000 0
> >3  0000:0000:0000:0000:0000:0000:0000:0000/000 0
> >4  0000:0000:0000:0000:0000:0000:0000:0000/000 0
> >5  0000:0000:0000:0000:0000:0000:0000:0000/000 0
> >6  0000:0000:0000:0000:0000:0000:0000:0000/000 0
> >7  0000:0000:0000:0000:0000:0000:0000:0000/000 0
> >8  0000:0000:0000:0000:0000:0000:0000:0000/000 0
> >9  0000:0000:0000:0000:0000:0000:0000:0000/000 0
> >10 0000:0000:0000:0000:0000:0000:0000:0000/000 0
> >11 0000:0000:0000:0000:0000:0000:0000:0000/000 0
> >12 0000:0000:0000:0000:0000:0000:0000:0000/000 0
> >13 0000:0000:0000:0000:0000:0000:0000:0000/000 0
> >14 0000:0000:0000:0000:0000:0000:0000:0000/000 0
> >15 0000:0000:0000:0000:0000:0000:0000:0000/000 0
> >
> >For setting a context e.g. context id 0, context 2001::, prefix-length
> >64.
> >
> >Hint: Simple copy one line and then maniuplate it.
> >
> >echo "0 2001:0000:0000:0000:0000:0000:0000:0000/64 1" >
> >/sys/kernel/debug/6lowpan/lowpan0/dci_table
> >
> >The state will display currently if the context is disabled(0) or
> >enabled(1).
> >
> >On transmit side:
> >
> >The IPHC code will automatically search for a context which would be
> >match for the address. Then it will be use the context which the
> >best compression, means the longest prefix which match will be used.
> >
> >Example:
> >
> >2001::/126 vs 2001::/127 - the 2001::/127 can be full compressed if the
> >last bit of the address which has the prefix 2001::/127 is the same like
> >the IID from the Encapsulating Header. A context ID can also be a
> >2001::1/128, which is then a full ipv6 address.
> >
> >On Receive side:
> >
> >If there is a context defined (when CID not available then it's the
> >default context 0) then it will be used, if the header doesn't set
> >SAC or DAC bit thens, it will be dropped.
> >
> >Signed-off-by: Alexander Aring <alex.aring-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >---
> >  include/net/6lowpan.h   |  62 ++++++
> >  net/6lowpan/6lowpan_i.h |   3 +
> >  net/6lowpan/Kconfig     |   1 +
> >  net/6lowpan/core.c      |  17 ++
> >  net/6lowpan/debugfs.c   | 109 +++++++++++
> >  net/6lowpan/iphc.c      | 500 ++++++++++++++++++++++++++++++++++++++++++------
> >  6 files changed, 635 insertions(+), 57 deletions(-)
> >
> >diff --git a/include/net/6lowpan.h b/include/net/6lowpan.h
> >index e484898..6e8d30e 100644
> >--- a/include/net/6lowpan.h
> >+++ b/include/net/6lowpan.h
> >@@ -75,6 +75,8 @@
> >  #define LOWPAN_IPHC_MAX_HC_BUF_LEN	(sizeof(struct ipv6hdr) +	\
> >  					 LOWPAN_IPHC_MAX_HEADER_LEN +	\
> >  					 LOWPAN_NHC_MAX_HDR_LEN)
> >+/* SCI/DCI is 4 bit width, so we have maximum 16 entries */
> >+#define LOWPAN_IPHC_CI_TABLE_SIZE	(1 << 4)
> >  #define LOWPAN_DISPATCH_IPV6		0x41 /* 01000001 = 65 */
> >  #define LOWPAN_DISPATCH_IPHC		0x60 /* 011xxxxx = ... */
> >@@ -98,9 +100,45 @@ enum lowpan_lltypes {
> >  	LOWPAN_LLTYPE_IEEE802154,
> >  };
> >+enum lowpan_iphc_ctx_states {
> >+	LOWPAN_IPHC_CTX_STATE_DISABLED,
> >+	LOWPAN_IPHC_CTX_STATE_ENABLED,
> >+
> >+	__LOWPAN_IPHC_CTX_STATE_INVALID,
> >+	LOWPAN_IPHC_CTX_STATE_MAX = __LOWPAN_IPHC_CTX_STATE_INVALID - 1,
> >+};
> >+
> 
> You are expecting more states here besides enabled and disabled? Because
> normally a bool would be fine here and an enum overkill. If you have more
> states pending it makes sense to have the enum though.
> 

Yea, I thought about more states than just these ones. E.g. from which
sources came the context which was set, from ICMPv6 option field, manually
added from debugfs/netlink? Such things...

Nevertheless I will change it to bool here, we can still change it if we
want that. But then we need to talk about that again if doing userspace
API for that.

> >+struct lowpan_iphc_ctx {
> >+	enum lowpan_iphc_ctx_states state;
> >+	u8 id;
> >+	struct in6_addr addr;
> >+	u8 prefix_len;
> >+};
> >+
...
> >  #include <linux/netdevice.h>
> >+extern const struct lowpan_iphc_ctx_ops iphc_ctx_unicast_ops;
> >+extern const struct lowpan_iphc_ctx_ops iphc_ctx_mcast_ops;
> >+
> >  #ifdef CONFIG_6LOWPAN_DEBUGFS
> >  int lowpan_dev_debugfs_init(struct net_device *dev);
> >  void lowpan_dev_debugfs_uninit(struct net_device *dev);
> >diff --git a/net/6lowpan/Kconfig b/net/6lowpan/Kconfig
> >index 01c901c..7ecedd7 100644
> >--- a/net/6lowpan/Kconfig
> >+++ b/net/6lowpan/Kconfig
> >@@ -8,6 +8,7 @@ menuconfig 6LOWPAN
> >  config 6LOWPAN_DEBUGFS
> >  	bool "6LoWPAN debugfs support"
> >  	depends on 6LOWPAN
> >+	depends on DEBUG_FS
> 
> This looks like a fix that should be merged into the first patch, no?

yes. I will fix that.

> >  	---help---
> >  	  This enables 6LoWPAN debugfs support. For example to manipulate
> >  	  IPHC context information at runtime.
> >diff --git a/net/6lowpan/core.c b/net/6lowpan/core.c
> >index fe58509..d354c5b 100644
> >--- a/net/6lowpan/core.c
> >+++ b/net/6lowpan/core.c
> >@@ -19,6 +19,8 @@
> >  int lowpan_netdev_setup(struct net_device *dev, enum lowpan_lltypes lltype)
> >  {
> >+	int i;
> >+
> >  	dev->addr_len = EUI64_ADDR_LEN;
> >  	dev->type = ARPHRD_6LOWPAN;
> >  	dev->mtu = IPV6_MIN_MTU;
...
> >+static ssize_t lowpan_context_dbgfs_write(struct file *fp,
> >+					  const char __user *user_buf,
> >+					  size_t count, loff_t *ppos)
> >+{
> >+	char buf[128] = { };
> >+	struct seq_file *file = fp->private_data;
> >+	struct lowpan_iphc_ctx_table *t = file->private;
> >+	struct lowpan_iphc_ctx ctx;
> >+	int status = count, n, id, state, i, prefix_len, ret;
> >+	unsigned int addr[8];
> >+
> >+	if (copy_from_user(&buf, user_buf, min_t(size_t, sizeof(buf) - 1,
> >+						 count))) {
> >+		status = -EFAULT;
> >+		goto out;
> >+	}
> >+
> >+	n = sscanf(buf, "%d %04x:%04x:%04x:%04x:%04x:%04x:%04x:%04x/%d %d",
> >+		   &id, &addr[0], &addr[1], &addr[2], &addr[3], &addr[4],
> >+		   &addr[5], &addr[6], &addr[7], &prefix_len, &state);
> >+	if (n != 11) {
> 
> 11 more like a magic number here. Not to bad, but a define could be nice.

ok.

...
> >+
> >+const struct lowpan_iphc_ctx_ops iphc_ctx_mcast_ops = {
> >+	.valid_prefix = lowpan_iphc_mcast_ctx_valid_prefix,
> >+	.update = lowpan_iphc_ctx_update,
> >+	.get_by_id = lowpan_iphc_ctx_get_by_id,
> >+	.get_by_addr = lowpan_iphc_ctx_get_by_mcast_addr,
> >+};
> 
> These are the comments from a first look over it. I will have a more
> detailed review and actual testing once you are happy enough with it to move
> it from RFC to PATCH.
> 

ok.

- Alex